-
Notifications
You must be signed in to change notification settings - Fork 688
feat: dashboard variables support in tabs and panel level #7578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
5c8b39a to
737b5a7
Compare
737b5a7 to
adc1ebe
Compare
79b664c to
724eb21
Compare
c36c676 to
1f8f009
Compare
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 81 | 76 | 1 | 2 | 2 | 94% | 24m 10s |
Test Failure Analysis
- alerts-import.spec.js: Visibility assertion failure due to multiple matching elements
- Alerts Import/Export Destination Import from URL and File: 'Successfully imported' resolved to 2 elements, causing strict mode violation.
Root Cause Analysis
- The recent changes in VariablesValueSelector.vue may have introduced multiple notifications that conflict with the visibility assertion in the test.
Recommended Actions
- Modify the test to specify a more unique text or selector to avoid ambiguity in alerts-import.spec.js. 2. Review the logic in VariablesValueSelector.vue to ensure only one notification is displayed for successful imports. 3. Implement a wait or check for the specific notification type before asserting visibility in the test.
1f8f009 to
5f627b4
Compare
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 323 | 295 | 1 | 17 | 10 | 91% | 4m 59s |
Test Failure Analysis
- dashboard-variables-setting.spec.js: Timeout errors due to element visibility and stability issues
- dashboard variables settings should verify that hide on constant variable dashboard is working: Element '[data-test="dashboard-settings-close-btn"]' not stable, causing click timeout.
Root Cause Analysis
- The recent changes in VariablesValueSelector.vue may have affected the stability of the dashboard settings close button.
Recommended Actions
- Investigate the visibility and stability of the button '[data-test="dashboard-settings-close-btn"]' in dashboard-settings.js. 2. Increase the timeout duration for the click action in the test to accommodate potential delays. 3. Ensure that the DOM is fully rendered before attempting to click the button in the test.
5f627b4 to
e560000
Compare
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 250 | 230 | 1 | 15 | 4 | 92% | 4m 32s |
Test Failure Analysis
- dashboard-variables-setting.spec.js: Timeout issues while interacting with elements
- dashboard variables settings should verify textbox variable by adding and verify that its visible on dashboard: Locator timeout - button[data-test="dashboard-settings-close-btn"] not stable and detached from DOM.
Root Cause Analysis
- The recent changes in VariablesValueSelector.vue may have affected the stability of the dashboard settings close button, leading to timeout errors.
Recommended Actions
- Investigate the stability of the button with data-test="dashboard-settings-close-btn" in VariablesValueSelector.vue. 2. Increase the timeout duration for click actions in dashboard-variables-setting.spec.js. 3. Ensure that the button is not detached from the DOM during the test execution.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 157 | 141 | 1 | 8 | 7 | 90% | 3m 2s |
Test Failure Analysis
- dashboard.spec.js: Assertion failure due to incorrect data count in chart
- dashboard UI testcases should update the line chart correctly when used camel case in custom sql query: Expected canvas count to be greater than 0 but received 0.
Root Cause Analysis
- The changes in web/.../dashboards/[VUE] VariablesValueSelector.vue may have affected the data flow, resulting in a canvas count of 0.
Recommended Actions
- Review the data fetching logic in web/.../dashboards/[VUE] VariablesValueSelector.vue to ensure it populates the canvas correctly.
- Add additional logging in the data processing functions to trace the data flow and identify where the count becomes 0.
- Validate the SQL query used in the test to ensure it returns the expected results when using camel case.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 3 tests failed | 231 | 212 | 3 | 14 | 2 | 92% | 4m 44s |
Test Failure Analysis
- dashboard-variables-setting.spec.js: All tests failed due to timeout errors on element interactions
- dashboard variables settings should verify that hide on textbox variable dashboard is working: Timeout while clicking close button, element unstable.
- dashboard variables settings should verify textbox variable by adding and verify that its visible on dashboard: Timeout while clicking close button, element unstable.
- dashboard variables settings should verify that hide on constant variable dashboard is working: Timeout while clicking close button, element unstable.
Root Cause Analysis
- The failures are likely related to the changes in dashboard settings interactions in dashboard-settings.js.
Recommended Actions
- Investigate the stability of the element with data-test="dashboard-settings-close-btn" in dashboard-settings.js. 2. Increase the timeout duration for the click actions in the tests to accommodate potential delays. 3. Ensure the element is present and stable before attempting to click, possibly by adding explicit waits.
2981c40 to
5a3c91a
Compare
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 3 tests failed | 140 | 128 | 3 | 6 | 3 | 91% | 4m 28s |
Test Failure Analysis
- dashboard-variables-setting.spec.js: All tests fail due to timeout issues with element visibility
- dashboard variables settings should verify that hide on textbox variable dashboard is working: Timeout waiting for button[data-test="dashboard-settings-close-btn"] to be clickable.
- dashboard variables settings should verify textbox variable by adding and verify that its visible on dashboard: Timeout waiting for button[data-test="dashboard-settings-close-btn"] to be clickable.
- dashboard variables settings should verify that hide on constant variable dashboard is working: Timeout waiting for button[data-test="dashboard-settings-close-btn"] to be clickable.
Root Cause Analysis
- The recent changes in VariablesValueSelector.vue may have introduced instability in the dashboard settings UI, causing timeouts.
Recommended Actions
- Investigate the stability of the button[data-test="dashboard-settings-close-btn"] in the DOM during test execution. 2. Add explicit waits or checks for visibility before attempting clicks in dashboard-variables-setting.spec.js. 3. Review the logic in VariablesValueSelector.vue to ensure it does not interfere with UI rendering.
1220aca to
3a16c81
Compare
| : variable.value; | ||
|
|
||
| const placeholders = [ | ||
| `\${${variable.name}}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, the code that escapes $, {, and } in the placeholders before constructing a RegExp pattern should use double backslashes ("\\$") in the replacement strings, so that the resultant string has a literal backslash before the meta-character, making it a valid RegExp escape inside the RegExp constructor.
Edit lines 248, 249, and 250 in web/src/utils/dashboard/variables/variablesUtils.ts:
- Change
.replace(/\$/g, "\\$")to.replace(/\$/g, "\\\$") - Change
.replace(/\{/g, "\\{")to.replace(/\{/g, "\\{")(unchanged, since{needs a single backslash and this is correct) - Change
.replace(/\}/g, "\\}")to.replace(/\}/g, "\\}")(unchanged, since}needs a single backslash and this is correct)
But wait: Are { and } escaping correct? The same logic applies—when building RegExp pattern strings, it is best to use double backslashes for escaping inside string, which results in a single backslash in the actual RegExp pattern.
So, change:
.replace(/\$/g, "\\$")to.replace(/\$/g, "\\\$").replace(/\{/g, "\\{")to.replace(/\{/g, "\\\{").replace(/\}/g, "\\}")to.replace(/\}/g, "\\\}")
This will ensure that the resulting RegExp pattern string has escaped characters for $, {, and } as needed to safely and literally match the placeholder strings.
No imports or additional dependencies are required.
-
Copy modified lines R248-R250
| @@ -245,9 +245,9 @@ | ||
| processedContent = processedContent.replace( | ||
| new RegExp( | ||
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), | ||
| .replace(/\$/g, "\\\$") | ||
| .replace(/\{/g, "\\\{") | ||
| .replace(/\}/g, "\\\}"), | ||
| "g", | ||
| ), | ||
| value, |
|
|
||
| const placeholders = [ | ||
| `\${${variable.name}}`, | ||
| `\${${variable.name}:csv}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, ensure that meta-characters $, {, and } are properly escaped when building a RegExp from a string literal. In JavaScript, this means using double backslashes in string literals so that the final string fed to new RegExp has a single backslash, as required by RegExp syntax. This affects lines 248-250:
- Replace
.replace(/\$/g, "\\$")with.replace(/\$/g, "\\\$") - Replace
.replace(/\{/g, "\\{")with.replace(/\{/g, "\\{")(curly brace does not always need escaping unless used in quantifiers, but generally safe to escape as\\{) - Replace
.replace(/\}/g, "\\}")with.replace(/\}/g, "\\}")(same as above)
The most important fix is to ensure that $ is properly escaped as \\$ (string literal), resulting in a single \$ in the RegExp.
Additionally, if any other characters with special meaning in RegExp (like ., *, ^, $, +, ?, (, ), [, ], {, }, |, \) could appear in the variable names or placeholders, consider a generic escape approach (using a utility like escapeRegExp). However, as per the snippet, only $, {, and } are targeted.
No new imports are required; this can be implemented with string replace.
-
Copy modified line R248
| @@ -245,7 +245,7 @@ | ||
| processedContent = processedContent.replace( | ||
| new RegExp( | ||
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\$/g, "\\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), | ||
| "g", |
| const placeholders = [ | ||
| `\${${variable.name}}`, | ||
| `\${${variable.name}:csv}`, | ||
| `\${${variable.name}:pipe}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 8 hours ago
The best way to fix the problem is to remove the unnecessary backslash before $ in the string literal on line 224 (\$${variable.name}). Change it to $${variable.name} so the string matches the dollar sign literally, as intended, without a redundant escape that only makes the code harder to read and possibly misleading.
Make this change only in the construction of the placeholders array, specifically on line 224. No other changes, imports, or definitions are required.
-
Copy modified line R224
| @@ -221,7 +221,7 @@ | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, | ||
| `\$${variable.name}`, | ||
| `$${variable.name}`, | ||
| ]; | ||
|
|
||
| placeholders.forEach((placeholder) => { |
| `\${${variable.name}}`, | ||
| `\${${variable.name}:csv}`, | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 8 hours ago
To fix the problem, remove the unnecessary backslash from the dollar sign in the placeholder template on line 224 ('\$' → '$'). This ensures the placeholder string is actually as intended ($variable), and any required escaping for the RegExp string pattern is correctly accomplished by the subsequent replacement step (which replaces all $ with \\$). Only the line constructing the placeholders array needs to be updated in web/src/utils/dashboard/variables/variablesUtils.ts, changing the fifth item (line 224) to use a plain dollar sign. No other changes or imports are required.
-
Copy modified line R224
| @@ -221,7 +221,7 @@ | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, | ||
| `\$${variable.name}`, | ||
| `$${variable.name}`, | ||
| ]; | ||
|
|
||
| placeholders.forEach((placeholder) => { |
| `\${${variable.name}:csv}`, | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, remove the unnecessary backslash escaping in the placeholder string on line 224, changing \$${variable.name} to simply $${variable.name}. Since the code that generates the RegExp pattern string already escapes $ characters on line 248 via .replace(/\$/g, "\\$"), the literal $ will be properly handled in the final RegExp pattern. No other changes are needed, as the escaping logic after this will ensure correctness in the final regular expression. Edit line 224 in web/src/utils/dashboard/variables/variablesUtils.ts, replacing \$${variable.name}, with $${variable.name},.
-
Copy modified line R224
| @@ -221,7 +221,7 @@ | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, | ||
| `\$${variable.name}`, | ||
| `$${variable.name}`, | ||
| ]; | ||
|
|
||
| placeholders.forEach((placeholder) => { |
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, | ||
| `\$${variable.name}`, |
Check failure
Code scanning / CodeQL
Useless regular-expression character escape High
regular expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix this issue, we should remove the unnecessary escape character before $ in the placeholder string array on line 224. That is, change \$${variable.name} to $${variable.name} in the placeholders array.
This fix should be made directly at the array construction, as the subsequent code already ensures the correct escaping of $ when building the regular expression pattern. No other code, imports, or refactoring is required.
Change the file web/src/utils/dashboard/variables/variablesUtils.ts, line 224:
- from:
\${variable.name} - to:
$${variable.name}
-
Copy modified line R224
| @@ -221,7 +221,7 @@ | ||
| `\${${variable.name}:pipe}`, | ||
| `\${${variable.name}:doublequote}`, | ||
| `\${${variable.name}:singlequote}`, | ||
| `\$${variable.name}`, | ||
| `$${variable.name}`, | ||
| ]; | ||
|
|
||
| placeholders.forEach((placeholder) => { |
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best fix is to properly escape all RegExp special characters when building a regex from any user-controlled or dynamic string. The standard way to do this is to use a function that escapes all special regex meta-characters—including backslash—in the placeholder string before passing it to new RegExp. This is best achieved by defining an escapeRegExp helper function, such as:
function escapeRegExp(string: string): string {
return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
}Then, replace the current manual escaping with a call to this helper. This change should be made only within the region where the RegExp is constructed—specifically, replace the chained .replace statements on lines 248–250 with a call to escapeRegExp.
Add the new helper function at an appropriate location in the file (often at the top or with other utilities).
-
Copy modified lines R1-R5 -
Copy modified line R252
| @@ -1,3 +1,8 @@ | ||
| // Utility to escape regex special characters in a dynamic string | ||
| function escapeRegExp(string: string): string { | ||
| return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } | ||
|
|
||
| export const formatInterval = (interval: any) => { | ||
| switch (true) { | ||
| // 0.01s | ||
| @@ -244,10 +249,7 @@ | ||
|
|
||
| processedContent = processedContent.replace( | ||
| new RegExp( | ||
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), | ||
| escapeRegExp(placeholder), | ||
| "g", | ||
| ), | ||
| value, |
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix the issue is to robustly escape all RegExp meta-characters in the dynamic placeholder string before using it in new RegExp(placeholder, "g"). The core issue is that the code currently uses a series of replace calls to escape certain characters, but misses escaping the backslash itself and any other RegExp special characters. The most robust solution is to use a well-tested escape function, such as escape-string-regexp from npm. Since we may only edit code we've been shown, we can safely add a well-known minimal helper function for escaping RegExp special characters.
For this file, add a local utility function escapeRegExp above this block, and then use it to escape placeholder before passing to RegExp. Remove the custom .replace(/\$/g, "\\$").replace(/\{/g, ...) logic and instead use escapeRegExp(placeholder).
No external npm dependencies are required for the minimal robust escape function. Only this region (lines 245-253) is changed, with the helper inserted earlier in the file.
-
Copy modified lines R1-R6 -
Copy modified line R253
| @@ -1,3 +1,9 @@ | ||
| // Escapes special characters for use in a regular expression | ||
| function escapeRegExp(string: string): string { | ||
| // From MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping | ||
| return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } | ||
|
|
||
| export const formatInterval = (interval: any) => { | ||
| switch (true) { | ||
| // 0.01s | ||
| @@ -244,10 +250,7 @@ | ||
|
|
||
| processedContent = processedContent.replace( | ||
| new RegExp( | ||
| placeholder | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), | ||
| escapeRegExp(placeholder), | ||
| "g", | ||
| ), | ||
| value, |
| placeholder | ||
| .replace(/\$/g, "\\$") |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix the problem is to ensure that all occurrences of backslash in the placeholder string are escaped before constructing the dynamic regular expression. This can be done by adding a .replace(/\\/g, "\\\\") call to the chain before the other replacements. Thus, the code should replace placeholder's backslashes to double backslashes, so that the resulting regex is correct. The fix is limited to the region of lines 246-250 in web/src/utils/dashboard/variables/variablesUtils.ts.
-
Copy modified line R248
| @@ -245,6 +245,7 @@ | ||
| processedContent = processedContent.replace( | ||
| new RegExp( | ||
| placeholder | ||
| .replace(/\\/g, "\\\\") | ||
| .replace(/\$/g, "\\$") | ||
| .replace(/\{/g, "\\{") | ||
| .replace(/\}/g, "\\}"), |
a3b1996 to
15117e1
Compare
… management - Added lazy loading capability to VariablesValueSelector for improved performance. - Introduced a new composable `useVariablesLoadingManager` to handle cascade decisions and dependency resolution. - Updated RenderDashboardCharts.vue to support lazy loading for both global and tab-level variables. - Implemented visibility tracking for panels to trigger loading of variables only when needed. - Enhanced logging for better debugging and tracking of variable updates and dependencies.
… in RenderDashboardCharts
ea4daeb to
0d3bed4
Compare
PR Type
Enhancement
Description
Add
scope,tabs,panelsfields in backend structUI to select variable scope and choose tabs/panels
Logic watches and filters selected tabs/panels
Display scope badges and tooltips in settings table
Changes diagram
Changes walkthrough 📝
mod.rs
Add scope, tabs, panels to VariableListsrc/config/src/meta/dashboards/v5/mod.rs
scopefield toVariableListtabsandpanelsarrays for scopingAddSettingVariable.vue
Add scoped tabs/panels UI and logicweb/src/components/dashboards/settings/AddSettingVariable.vue
selectedTabs,selectedPanels,scopeOptionsupdatePanelsfilter logicvariableData.tabsandvariableData.panelson saveVariableSettings.vue
Show variable scope badges in settingsweb/src/components/dashboards/settings/VariableSettings.vue
getScopeType,getTabName,getPanelName