Skip to content

Conversation

@ktx-vaidehi
Copy link
Collaborator

@ktx-vaidehi ktx-vaidehi commented Aug 29, 2025

PR Type

Bug fix


Description

  • Prevent refresh warning without dashboard variables

  • Reset refreshed variables state when none exist


Diagram Walkthrough

flowchart LR
  A["Dashboard load"] -- "No variables present" --> B["Set variablesData to idle"]
  A -- "No variables present" --> C["Set refreshedVariablesData to idle"]
  B -- "Prevents spurious warnings" --> D["Clean UX"]
  C -- "Avoid stale refreshed state" --> D
Loading

File Walkthrough

Relevant files
Bug fix
ViewDashboard.vue
Reset refreshed variables state when none exist                   

web/src/views/Dashboards/ViewDashboard.vue

  • When no variables exist, also reset refreshedVariablesData.
  • Set refreshedVariablesData.isVariablesLoading to false.
  • Clear refreshedVariablesData.values array.
+2/-0     

@ktx-vaidehi ktx-vaidehi marked this pull request as ready for review August 29, 2025 05:49
@github-actions github-actions bot added ☢️ Bug Something isn't working Review effort 2/5 labels Aug 29, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2025

PR Reviewer Guide 🔍

(Review updated until commit b9667ec)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

State Consistency

Ensure resetting both variablesData and refreshedVariablesData to non-loading empty states is consistent with downstream consumers (watchers/computed/conditions) that may expect null/undefined or prior shape; verify no components rely on previous values or loading transitions.

  variablesData.isVariablesLoading = false;
  variablesData.values = [];
  refreshedVariablesData.isVariablesLoading = false;
  refreshedVariablesData.values = [];
}
UX Edge Case

Confirm that setting isVariablesLoading to false immediately when no variables exist does not skip any intended skeleton/loading cues or trigger conditions that assume a prior loading phase.

  variablesData.isVariablesLoading = false;
  variablesData.values = [];
  refreshedVariablesData.isVariablesLoading = false;
  refreshedVariablesData.values = [];
}

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2025

PR Code Suggestions ✨

Latest suggestions up to b9667ec
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null guard on assignment

Guard against refreshedVariablesData being undefined to avoid runtime errors in edge
cases when the object isn't initialized. Add optional chaining or a null check
before assigning.

web/src/views/Dashboards/ViewDashboard.vue [689-690]

-refreshedVariablesData.isVariablesLoading = false;
-refreshedVariablesData.values = [];
+if (refreshedVariablesData) {
+  refreshedVariablesData.isVariablesLoading = false;
+  refreshedVariablesData.values = [];
+}
Suggestion importance[1-10]: 4

__

Why: The existing lines at 689-690 match and adding a guard could prevent a runtime error if refreshedVariablesData is ever undefined, but the PR context likely implies it exists; impact is minor and situational.

Low

Previous suggestions

Suggestions up to commit b9667ec
CategorySuggestion                                                                                                                                    Impact
General
Ensure reactive array updates

Ensure reactive updates by replacing the arrays instead of mutating them directly.
In Vue reactivity, reassigning to a new array helps avoid stale references and
ensures watchers/computed values update correctly.

web/src/views/Dashboards/ViewDashboard.vue [687-690]

 variablesData.isVariablesLoading = false;
 variablesData.values = [];
 refreshedVariablesData.isVariablesLoading = false;
-refreshedVariablesData.values = [];
+refreshedVariablesData.values = [...[]];
Suggestion importance[1-10]: 2

__

Why: The proposed change is effectively a no-op: [] and [...[]] both create new arrays, offering no improvement in Vue reactivity. The existing code already replaces arrays, so the suggestion adds little value and does not address an actual issue.

Low

@github-actions
Copy link
Contributor

Persistent review updated to latest commit b9667ec

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR fixes a UX issue in the dashboard interface where the refresh button would incorrectly display a warning state (orange color with warning message) when no variables are present on the dashboard. The problem occurred because of inconsistent initialization of two reactive objects used to track variable states.

The change is in ViewDashboard.vue and addresses the root cause by ensuring both variablesData and refreshedVariablesData reactive objects are properly initialized with identical empty states when a dashboard has no variables configured. Specifically, when no variables are present, both objects now get:

  • isVariablesLoading set to false
  • values set to an empty array []

This fix integrates with the existing isVariablesChanged computed property that performs deep equality comparison between these two objects to determine whether to show the warning state. Previously, only variablesData was being initialized while refreshedVariablesData remained uninitialized (empty object {}), causing the comparison to always return true for "variables changed" even when no variables existed. The solution ensures consistent initialization, making the comparison accurate and eliminating false positive warnings.

The change fits well within the component's architecture, which already handles various dashboard states and uses these reactive objects throughout the variable management system.

Confidence score: 5/5

  • This PR is safe to merge with minimal risk
  • Score reflects a targeted bug fix with clear logic and no side effects
  • No files require special attention

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@ktx-vaidehi ktx-vaidehi force-pushed the fix/dashboard/refresh-warning-when-no-variables-present branch from b9667ec to 352bc06 Compare August 29, 2025 07:58
@ktx-vaidehi ktx-vaidehi merged commit 4ce6a16 into main Aug 29, 2025
29 of 30 checks passed
@ktx-vaidehi ktx-vaidehi deleted the fix/dashboard/refresh-warning-when-no-variables-present branch August 29, 2025 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

☢️ Bug Something isn't working Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants