Skip to content

Conversation

@denisov-vlad
Copy link
Member

@denisov-vlad denisov-vlad commented Nov 12, 2025

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

Added support for saving dashboard parameters after clicking the Apply button. This is useful when using query templates and inserting them into different dashboards. Without this logic, there are two solutions: cloning queries or making parameters static.

Now they are used in the following order: url, dashboard parameters, query parameters.

  • Queue dashboard-level parameter edits while the dashboard is in edit mode.
  • Persist the queued values only when “Done Editing” is clicked, keeping Query and Dashboard editors aligned.
  • Surface any save failures via notifications and restore the queued values so nothing is lost silently.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A
  1. Create query with params
  2. Create dashboard and add query widget there
  3. Check the default value and change it
  4. Open new dashboard from dashboards list
  5. You'll see changed value

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@denisov-vlad denisov-vlad marked this pull request as ready for review November 12, 2025 11:40
@denisov-vlad
Copy link
Member Author

I skipped the Restyled check to give the reviewer a chance to look at the code that was changed.

I would recommend running yarn prettier separately to change all files in the repository.

@yoshiokatsuneo
Copy link
Contributor

@denisov-vlad

Thank you for your PR !
I also see this issue, and feel annoying. It is cool that you are trying to fix the issue !

I think the parameters should be saved only when "Done Editing" button is clicked, as the "Done Editing" button is for saving, and it makes the behavior of the Query Editor and the Dashboard Editor consistent.

@arikfr arikfr requested a review from Copilot November 14, 2025 06:55
Copilot finished reviewing on behalf of arikfr November 14, 2025 06:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the ability to persist dashboard parameter values when the Apply button is clicked. This addresses the need to maintain customized parameter values across dashboard sessions without requiring query cloning or static parameters. The implementation establishes a new parameter precedence order: URL parameters override dashboard-saved parameters, which in turn override query-defined default parameters.

  • Introduces parameter value persistence in dashboard.options.parameterValues
  • Implements persistParameterValues function to save parameter updates to the backend
  • Updates parameter loading to check for saved values before applying defaults

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
client/app/services/dashboard.js Modified getParametersDefs to load saved parameter values from dashboard options and apply them before URL parameters
client/app/pages/dashboards/hooks/useDashboard.js Added persistParameterValues function and integrated it into the refreshDashboard flow with error handling
client/app/pages/dashboards/DashboardPage.jsx Fixed onParametersEdit to preserve existing dashboard options when updating parameter order

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 188 to 192
.catch(error => {
console.error("Failed to persist parameter values:", error);
notification.error("Parameter values could not be saved. Your changes may not be persisted.");
})
.then(() => loadDashboard(true, updatedParameters))
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The error in persistParameterValues is caught but the promise chain continues to loadDashboard even if persistence fails. This means the dashboard will refresh with new values that haven't been saved. Consider returning early or skipping the refresh if persistence fails to prevent showing unsaved state.

Suggested change
.catch(error => {
console.error("Failed to persist parameter values:", error);
notification.error("Parameter values could not be saved. Your changes may not be persisted.");
})
.then(() => loadDashboard(true, updatedParameters))
.then(() => {
// Only refresh dashboard if persistence succeeded
loadDashboard(true, updatedParameters);
})
.catch(error => {
console.error("Failed to persist parameter values:", error);
notification.error("Parameter values could not be saved. Your changes may not be persisted.");
// Do not refresh dashboard if persistence failed
})

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Promise.resolve(persistParameterValues(pendingValues)).catch(error => {
console.error("Failed to persist parameter values:", error);
notification.error("Parameter values could not be saved. Your changes may not be persisted.");
pendingParameterValuesRef.current = pendingValues;
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The error handling here may not work as intended. The updateDashboard function (lines 68-96) catches errors but doesn't re-throw them, meaning the promise resolves successfully even when the save fails. As a result, this catch block will not execute for most failure scenarios (except for cases where Dashboard.save itself rejects), and users won't see the error message or have their pending values restored. Consider modifying the implementation to properly detect and handle save failures.

Suggested change
pendingParameterValuesRef.current = pendingValues;
pendingParameterValuesRef.current = pendingValues;
throw error;

Copilot uses AI. Check for mistakes.
Comment on lines 268 to 271
Promise.resolve(persistParameterValues(pendingValues)).catch(error => {
console.error("Failed to persist parameter values:", error);
notification.error("Parameter values could not be saved. Your changes may not be persisted.");
pendingParameterValuesRef.current = pendingValues;
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

There's a potential race condition if the user re-enters edit mode while a persist operation is still in flight. If the persist operation then fails and tries to restore values at line 271, it could overwrite the new edit session's (empty) pending values with old values from the previous session. Consider tracking an edit session ID or timestamp to ensure restored values belong to the current session.

Copilot uses AI. Check for mistakes.
@yoshiokatsuneo
Copy link
Contributor

@denisov-vlad

Thank you for updating.
And again, I think the parameters should be saved only when "Done Editing" button(Not "Apply Changes" button) is clicked, as the "Done Editing" button is for saving, and it makes the behavior of the Query Editor and the Dashboard Editor consistent.

@denisov-vlad
Copy link
Member Author

denisov-vlad commented Nov 14, 2025

@denisov-vlad

Thank you for updating. And again, I think the parameters should be saved only when "Done Editing" button(Not "Apply Changes" button) is clicked, as the "Done Editing" button is for saving, and it makes the behavior of the Query Editor and the Dashboard Editor consistent.

Hi @yoshiokatsuneo

Have you tested it? It saves changes only after "Done editing" button click. I had tried to find any other option to save params, but the only way I found is with that button.

Also added some info to the PR message.

@yoshiokatsuneo
Copy link
Contributor

@denisov-vlad

Thank you !
I'm sorry I was wrong. The PR is actually working as described.

And, I think refreshDashboard() function should just refresh dashboard as named. How about ?

@denisov-vlad
Copy link
Member Author

denisov-vlad commented Nov 17, 2025

@yoshiokatsuneo The logic is consistent with the queries. The parameters are updated in edit mode, but after clicking Apply Changes and then Done Editing.

I have now simplified the code a little and removed it from refreshDashboard.

@yoshiokatsuneo
Copy link
Contributor

@denisov-vlad

Thank you for updating. The behavior looks good !
And for about the code, I think it is a bit unclear from the code when the parameters are saved.

How about making it easier to understand ?
For example, how about saving the parameter on onclick event handler of "Done Editing" button so that people can see the flow ?

…boardEditControl for improved parameter persistence
@denisov-vlad
Copy link
Member Author

@yoshiokatsuneo I hope I understood you correctly. I'm not a developer and haven't explored the front-end code in depth, but I hope everything is okay now.

Copy link
Contributor

@yoshiokatsuneo yoshiokatsuneo left a comment

Choose a reason for hiding this comment

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

Thank you for the update !
I added comments.

);

const persistParameterValues = useCallback(
parameterValues => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just save parameterValues ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that it's possible right now. We must update options and there could be globalParamOrder key. With my approach we will save all the keys in this field.

…ters and merge parameter values for improved persistence
@denisov-vlad
Copy link
Member Author

Thank you for the update ! I added comments.

@yoshiokatsuneo I´ve pushed some changes again. Could you check it please?

…d remove unnecessary checks for improved clarity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants