feat: Multi Select Variable #3372
Conversation
ktx-vaidehi
force-pushed
the
feat/dashboard-variables-multi-select
branch
2 times, most recently
from
2d6fdc2
to
148eab6
Compare
ktx-vaidehi
changed the title
ktx-vaidehi
force-pushed
the
feat/dashboard-variables-multi-select
branch
7 times, most recently
from
8e48dd3
to
a7b3fee
Compare
ktx-vaidehi
changed the title
ktx-vaidehi
changed the title
ktx-vaidehi
force-pushed
the
feat/dashboard-variables-multi-select
branch
11 times, most recently
from
53c65c6
to
e39fd6b
Compare
ktx-vaidehi
force-pushed
the
feat/dashboard-variables-multi-select
branch
6 times, most recently
from
04bb875
to
2a7f0fe
Compare
ktx-abhay
force-pushed
the
feat/dashboard-variables-multi-select
branch
from
a014e12
to
f838534
Compare
WalkthroughThe recent changes encompass enhancements to variable handling, UI updates, and dependency upgrades. Key updates include the addition of multi-select functionality for variables, improvements in query parsing and filtering logic, and the introduction of new components for custom value selection. The Quasar package has been upgraded, and fullscreen capabilities have been integrated using Quasar's AppFullscreen module. These updates enhance the flexibility and functionality of the dashboard components. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (11)
web/src/main.ts (1)
Line range hint
52-52: Consider replacinganywith a more specific type to enhance type safety and maintainability.web/src/components/dashboards/addPanel/DrilldownPopUp.vue (1)
Line range hint
595-625: Consider renaminggetvariableNamestogetVariableNamesfor consistency with JavaScript naming conventions.web/src/composables/dashboard/usePanelDataLoader.ts (3)
Line range hint
37-41: Consider specifying more precise types instead ofany.The use of
anytype can lead to less predictable code and potential runtime errors. It's generally a good practice in TypeScript to use more specific types to take full advantage of TypeScript's type system for better code safety and clarity.Also applies to: 43-43, 50-50, 57-57, 60-60, 67-67, 68-68, 74-74, 95-95, 96-96, 98-98, 111-111, 141-141
Line range hint
94-97: Optimize array operations with.flatMap().- ?.map((it: any) => it?.value) - ?.flat() + ?.flatMap((it: any) => it?.value)Replacing
.map().flat()with.flatMap()simplifies the code and potentially improves performance by reducing the depth of method chaining.
Line range hint
114-114: Use strict equality checks.- timestamps.start_time != "Invalid Date" + timestamps.start_time !== "Invalid Date"Using
!==instead of!=ensures that the comparison is both value and type safe.web/src/composables/useDashboardPanel.ts (6)
Line range hint
36-36: Specify a more appropriate type instead ofanyforgetDefaultDashboardPanelData.- const getDefaultDashboardPanelData: any = () => ({ + const getDefaultDashboardPanelData: DashboardPanelData = () => ({Consider defining a
DashboardPanelDatatype that accurately represents the structure of the data returned by this function.
Line range hint
214-214: Use strict equality comparison (===) instead of loose equality (==).- () => dashboardPanelData.data.queryType == "promql" + () => dashboardPanelData.data.queryType === "promql"Using
===avoids type coercion and ensures that both the type and value must match, which is generally safer and more predictable in JavaScript.
Line range hint
307-307: Use strict equality comparison (===) instead of loose equality (==).- row.name == store.state.zoConfig.timestamp_column + row.name === store.state.zoConfig.timestamp_columnIt's best practice to use
===for comparisons to ensure both type and value equality, which prevents potential bugs related to type coercion.
Line range hint
320-324: Use template literals for string concatenation to improve readability and maintainability.- "x_axis_" + (dashboardPanelData.data.queries[dashboardPanelData.layout.currentQueryIndex].fields.x.length + 1) + `x_axis_${dashboardPanelData.data.queries[dashboardPanelData.layout.currentQueryIndex].fields.x.length + 1}`Template literals provide a more readable and concise way to handle dynamic string creation.
Line range hint
329-329: Ensure strict equality checks are used to avoid unintended type coercions.- dashboardPanelData.data.queryType == "sql" + dashboardPanelData.data.queryType === "sql" - dashboardPanelData.data.type == "table" + dashboardPanelData.data.type === "table" - dashboardPanelData.data.queries[dashboardPanelData.layout.currentQueryIndex].customQuery == false + dashboardPanelData.data.queries[dashboardPanelData.layout.currentQueryIndex].customQuery === falseUsing
===ensures that both the type and value are checked, which is more reliable and prevents errors that might occur due to type coercion.Also applies to: 333-334, 362-362, 384-384
848-848: Increase thesizeparameter to accommodate more data if needed.The
sizeparameter is set to100, which might be insufficient depending on the expected data volume. Consider increasing this value if larger datasets are commonly used.Also applies to: 892-892
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (18)
- src/common/meta/dashboards/v3/mod.rs (1 hunks)
- web/package.json (1 hunks)
- web/src/components/dashboards/PanelSchemaRenderer.vue (1 hunks)
- web/src/components/dashboards/VariablesValueSelector.vue (11 hunks)
- web/src/components/dashboards/addPanel/DashboardMapQueryBuilder.vue (2 hunks)
- web/src/components/dashboards/addPanel/DashboardQueryBuilder.vue (2 hunks)
- web/src/components/dashboards/addPanel/DashboardQueryEditor.vue (1 hunks)
- web/src/components/dashboards/addPanel/DashboardSankeyChartBuilder.vue (2 hunks)
- web/src/components/dashboards/addPanel/DrilldownPopUp.vue (5 hunks)
- web/src/components/dashboards/settings/AddSettingVariable.vue (5 hunks)
- web/src/components/dashboards/settings/VariableCustomValueSelector.vue (1 hunks)
- web/src/components/dashboards/settings/VariableQueryValueSelector.vue (4 hunks)
- web/src/composables/dashboard/usePanelDataLoader.ts (5 hunks)
- web/src/composables/useDashboardPanel.ts (2 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/main.ts (2 hunks)
- web/src/views/Dashboards/RenderDashboardCharts.vue (1 hunks)
- web/src/views/Dashboards/ViewDashboard.vue (4 hunks)
Files skipped from review due to trivial changes (2)
- web/package.json
- web/src/locales/languages/en.json
Additional Context Used
Biome (41)
web/src/composables/dashboard/usePanelDataLoader.ts (20)
37-37: Unexpected any. Specify a different type.
38-38: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
40-40: Unexpected any. Specify a different type.
41-41: Unexpected any. Specify a different type.
43-43: Unexpected any. Specify a different type.
50-50: Unexpected any. Specify a different type.
57-57: Unexpected any. Specify a different type.
60-60: Unexpected any. Specify a different type.
67-67: Unexpected any. Specify a different type.
67-67: Use !== instead of !=.
!= is only allowed when comparing againstnull
68-68: Unexpected any. Specify a different type.
74-74: Unexpected any. Specify a different type.
94-97: The call chain .map().flat() can be replaced with a single .flatMap() call.
95-95: Unexpected any. Specify a different type.
96-96: Unexpected any. Specify a different type.
98-98: Unexpected any. Specify a different type.
111-111: Unexpected any. Specify a different type.
114-114: Use === instead of ==.
== is only allowed when comparing againstnull
141-141: Unexpected any. Specify a different type.
web/src/composables/useDashboardPanel.ts (20)
36-36: Unexpected any. Specify a different type.
130-130: Unexpected any. Specify a different type.
135-135: Unexpected any. Specify a different type.
164-164: Unexpected any. Specify a different type.
214-214: Use === instead of ==.
== is only allowed when comparing againstnull
217-217: Unexpected any. Specify a different type.
245-245: Unexpected any. Specify a different type.
275-275: Unexpected any. Specify a different type.
288-288: Unexpected any. Specify a different type.
307-307: Unexpected any. Specify a different type.
307-307: Use === instead of ==.
== is only allowed when comparing againstnull
320-324: Template literals are preferred over string concatenation.
329-329: Use === instead of ==.
== is only allowed when comparing againstnull
333-333: Use === instead of ==.
== is only allowed when comparing againstnull
334-334: Use === instead of ==.
== is only allowed when comparing againstnull
344-344: Unexpected any. Specify a different type.
362-362: Unexpected any. Specify a different type.
362-362: Use === instead of ==.
== is only allowed when comparing againstnull
375-379: Template literals are preferred over string concatenation.
384-384: Use === instead of ==.
== is only allowed when comparing againstnullweb/src/main.ts (1)
52-52: Unexpected any. Specify a different type.
Additional comments not posted (21)
web/src/main.ts (2)
17-17: Import ofAppFullscreencorrectly added to support enhanced fullscreen functionality.
42-42: Properly configuredAppFullscreenin Quasar plugins to enable enhanced fullscreen capabilities.web/src/components/dashboards/settings/VariableQueryValueSelector.vue (3)
Line range hint
25-41: Implemented multi-select functionality in the template correctly, including UI controls for 'Select All' which enhances user interaction.Also applies to: 50-68
Line range hint
89-169: Script changes correctly implement the logic for multi-select functionality, including computed properties and methods for handling 'Select All' and displaying selected values.
39-41: Correctly added new attributes and event handlers to support multi-select functionality, ensuring the component is responsive and functional.Also applies to: 50-68
web/src/components/dashboards/settings/VariableCustomValueSelector.vue (1)
17-169: Introduced a new componentVariableCustomValueSelectorwhich is well-implemented with necessary logic for multi-select functionality. The component uses Vue features effectively and follows best practices.src/common/meta/dashboards/v3/mod.rs (1)
273-274: Addedmulti_selectfield to theVariableListstruct correctly, usingOption<bool>to support optional multi-select configuration.web/src/views/Dashboards/RenderDashboardCharts.vue (1)
274-274: Implemented debouncing for dashboard print mode effectively, ensuring smooth transition into print mode once all data is loaded.web/src/components/dashboards/addPanel/DrilldownPopUp.vue (1)
627-629: The modifications to the watch function are appropriate and ensure that variable names are updated when the folder or dashboard changes.web/src/components/dashboards/VariablesValueSelector.vue (2)
68-72: Successfully integratedVariableCustomValueSelectorfor custom variable types.The integration of
VariableCustomValueSelectoris correctly implemented with proper event handling for model value updates. This change aligns with the PR's objective to enhance multi-select functionality.
87-87: Ensure consistent component registration.This script checks for consistent registration of
VariableCustomValueSelectoracross all Vue components to ensure that it is available wherever it's used.Also applies to: 106-106
Verification successful
The output shows that
VariableCustomValueSelectoris used, imported, and registered in the components. The context provided indicates that it is consistently registered where it is used.
- The component is imported correctly.
- It is registered in the
componentssection.- It is used within the template.
This confirms that
VariableCustomValueSelectoris consistently registered across the components where it is used.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `VariableCustomValueSelector` is consistently registered across all components where it's used. # Test: Search for inconsistent component registrations. Expect: Consistent usage across all files. rg --type vue 'VariableCustomValueSelector'Length of output: 71
Script:
#!/bin/bash # Description: Verify that `VariableCustomValueSelector` is consistently registered across all components where it's used. # Find all .vue files and search for `VariableCustomValueSelector` within them. fd -e vue -x rg 'VariableCustomValueSelector' {}Length of output: 245
Script:
#!/bin/bash # Description: Verify that `VariableCustomValueSelector` is consistently registered across all components where it's used. # Find all .vue files and search for `VariableCustomValueSelector` within them, showing surrounding lines for context. fd -e vue -x rg -C 3 'VariableCustomValueSelector' {}Length of output: 1203
web/src/components/dashboards/PanelSchemaRenderer.vue (1)
425-432: Verify the impact of the updatedreplacePlaceholdersfunction on other parts of the application.web/src/components/dashboards/settings/AddSettingVariable.vue (2)
160-166: Ensure themultiSelecttoggle is properly integrated in the UI and functions as expected.Verification successful
The
multiSelecttoggle is indeed present and integrated within the Vue components, as evidenced by the search results. The toggle is referenced multiple times, indicating its implementation and usage in the codebase.
v-model="variableData.multiSelect":label="t('dashboard.multiSelect')"These references confirm that the
multiSelecttoggle is part of the UI and is being used as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `multiSelect` toggle is visible and functional in the UI. # Test: Search for the `multiSelect` toggle in the UI components. rg --type vue $'multiSelect'Length of output: 55
Script:
#!/bin/bash # Description: Verify the `multiSelect` toggle is visible and functional in the UI. # Find all Vue files and search for the `multiSelect` keyword within them. fd -e vue -x rg 'multiSelect' {}Length of output: 1270
303-309: Ensure themultiSelecttoggle for custom variables is properly integrated in the UI and functions as expected.Verification successful
The
multiSelecttoggle for custom variables is properly integrated in the UI and appears to be functional based on the search results.
- The
multiSelectproperty is used in various conditions and bindings within the Vue components.- The
v-modelbindings and labels formultiSelectare present, indicating that the toggle is correctly set up.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `multiSelect` toggle for custom variables is visible and functional in the UI. # Test: Search for the `multiSelect` toggle in the UI components. rg --type vue $'multiSelect'Length of output: 55
Script:
#!/bin/bash # Description: Verify the `multiSelect` toggle for custom variables is visible and functional in the UI. # Find all .vue files and search for the `multiSelect` toggle within them. fd -e vue -x rg 'multiSelect' {}Length of output: 1270
web/src/components/dashboards/addPanel/DashboardQueryEditor.vue (1)
791-810: Enhance query validation by replacing placeholders with dummy values.The addition of placeholder replacement with dummy values (
csv,singlequote,doublequote,pipe, and general variables) is a robust approach to validate the SQL query syntax before execution. This helps in catching syntax errors early in the development or testing phase, which can prevent runtime errors and improve the reliability of the query execution process.web/src/components/dashboards/addPanel/DashboardSankeyChartBuilder.vue (2)
909-911: LGTM! Ensure that the handling ofmultiSelectindashboardVariablesFilterItemsis consistent across all components that use this property.Verification successful
The handling of the
multiSelectproperty indashboardVariablesFilterItemsis consistent across the components:
DashboardMapQueryBuilder.vue,DashboardQueryBuilder.vue, andDashboardSankeyChartBuilder.vueuseit.multiSelectin a similar manner.AddSettingVariable.vueproperly initializes and usesmultiSelectwith default values and template bindings.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all components using `dashboardVariablesFilterItems` handle the `multiSelect` property consistently. # Test: Search for usage of `dashboardVariablesFilterItems`. Expect: Consistent handling of `multiSelect`. rg --type vue $'dashboardVariablesFilterItems'Length of output: 73
Script:
#!/bin/bash # Description: Verify that all components using `dashboardVariablesFilterItems` handle the `multiSelect` property consistently. # Test: Search for usage of `dashboardVariablesFilterItems` in .vue files. Expect: Consistent handling of `multiSelect`. rg 'dashboardVariablesFilterItems' --glob '*.vue'Length of output: 1485
Script:
#!/bin/bash # Description: Verify the handling of `multiSelect` property in components using `dashboardVariablesFilterItems`. # Test: Search for the term `multiSelect` in the identified .vue files. rg 'multiSelect' web/src/components/dashboards/settings/AddSettingVariable.vue web/src/components/dashboards/addPanel/DashboardSankeyChartBuilder.vue web/src/components/dashboards/addPanel/DashboardQueryBuilder.vue web/src/components/dashboards/addPanel/DashboardMapQueryBuilder.vueLength of output: 1429
948-948: LGTM! Verify that all UI components using theoptionsarray correctly handle the "IN" operator.web/src/components/dashboards/addPanel/DashboardMapQueryBuilder.vue (2)
918-920: Ensure correct handling of multi-select variables in filter expressions.The logic to handle multi-select variables by formatting them with braces and using the "IN" operator is correctly implemented. This allows for proper SQL query generation when multiple values are selected.
958-958: Validate the inclusion of the "IN" operator in the options list.Adding the "IN" operator to the list of comparison operators is appropriate for handling multi-select scenarios in SQL queries. This ensures that the UI allows users to select this operator when needed.
web/src/components/dashboards/addPanel/DashboardQueryBuilder.vue (2)
1321-1323: Ensure correct handling of multi-select variables in filter items.The logic to handle multi-select variables by formatting them differently in the filter items seems correct. This allows for proper query construction depending on whether a variable supports multi-selection.
1350-1350: Add "IN" operator to support multi-select functionality.Adding the "IN" operator is essential for supporting multi-select functionality in queries, allowing for the selection of multiple values in a single query condition. This is a necessary update for the new feature.
| @@ -65,11 +65,15 @@ export const usePanelDataLoader = ( | |||
| JSON.stringify( | |||
| variablesData.value?.values | |||
| ?.filter((it: any) => it.type != "dynamic_filters") // ad hoc filters are not considered as dependent filters as they are globally applied | |||
There was a problem hiding this comment.
Use strict equality checks.
- it.type != "dynamic_filters"
+ it.type !== "dynamic_filters"Using !== instead of != avoids type coercion and ensures that the comparison is both value and type safe.
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.
| ?.filter((it: any) => it.type != "dynamic_filters") // ad hoc filters are not considered as dependent filters as they are globally applied | |
| ?.filter((it: any) => it.type !== "dynamic_filters") // ad hoc filters are not considered as dependent filters as they are globally applied |
| if (!quasar.fullscreen.isActive) { | ||
| quasar.fullscreen | ||
| .request() | ||
| .then(() => { | ||
| isFullscreen.value = true; | ||
| }) | ||
| .catch(() => { | ||
| isFullscreen.value = false; | ||
| }); | ||
| } else { | ||
| quasar.fullscreen | ||
| .exit() | ||
| .then(() => { | ||
| isFullscreen.value = false; | ||
| }) | ||
| .catch(() => { | ||
| isFullscreen.value = true; | ||
| }); |
There was a problem hiding this comment.
Refactor fullscreen toggle logic for clarity and error handling.
- if (!quasar.fullscreen.isActive) {
- quasar.fullscreen
- .request()
- .then(() => {
- isFullscreen.value = true;
- })
- .catch(() => {
- isFullscreen.value = false;
- });
- } else {
- quasar.fullscreen
- .exit()
- .then(() => {
- isFullscreen.value = false;
- })
- .catch(() => {
- isFullscreen.value = true;
- });
+ try {
+ if (!quasar.fullscreen.isActive) {
+ await quasar.fullscreen.request();
+ isFullscreen.value = true;
+ } else {
+ await quasar.fullscreen.exit();
+ isFullscreen.value = false;
+ }
+ } catch (error) {
+ console.error('Error toggling fullscreen:', error);
+ isFullscreen.value = !isFullscreen.value; // Toggle to the opposite state on error
+ }This refactoring uses async/await for better readability and error handling. It logs the error and toggles the fullscreen state to the opposite value if an error occurs, ensuring the UI reflects the actual state.
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.
| if (!quasar.fullscreen.isActive) { | |
| quasar.fullscreen | |
| .request() | |
| .then(() => { | |
| isFullscreen.value = true; | |
| }) | |
| .catch(() => { | |
| isFullscreen.value = false; | |
| }); | |
| } else { | |
| quasar.fullscreen | |
| .exit() | |
| .then(() => { | |
| isFullscreen.value = false; | |
| }) | |
| .catch(() => { | |
| isFullscreen.value = true; | |
| }); | |
| try { | |
| if (!quasar.fullscreen.isActive) { | |
| await quasar.fullscreen.request(); | |
| isFullscreen.value = true; | |
| } else { | |
| await quasar.fullscreen.exit(); | |
| isFullscreen.value = false; | |
| } | |
| } catch (error) { | |
| console.error('Error toggling fullscreen:', error); | |
| isFullscreen.value = !isFullscreen.value; // Toggle to the opposite state on error | |
| } |
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
UI/UX Improvements
Localization
Performance
sizeparameter inuseDashboardPanelDatafrom 10 to 100 for better data handling.