-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Popup is getting automatically closed when there is a DAG running #57568
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
Conversation
|
I don't think this is the approach we should take. Instead we should make sure auto refresh doesn't affect modals. Maybe our mounting/unmounting approach needs to be reconsidered |
|
@bbovenzi Thank you for your suggestion. If I lift the state of the dialog box up to the page level, and the button only triggers it to open, with the page level rendering the dialog box uniformly, would this be a better approach? |
|
I don't know the technical details but I agree that the best behaviour would be that popup doesn't get closed but auto-refresh still continues on background. Even with the modal is opened, if a DAG finishes, we should see it state change in the background |
|
@matthieuauger Thank you for your feedback.I elevated the state to the page level and render the dialog box there as well. The button utilizes the onOpen event to trigger a state update on the page. This setup guarantees that refreshing the data only updates the table rows and leaves the dialog boxes unaffected. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is also encountered for other modals on the page. (mark task as failed, mark task as success).
And the problem is similar for the DagRuns table too.
|
Also this is probably not a scalable approach. The problem comes from the fact that the components are created on each render. And therefore the Basically |
|
Yes, I added this on the initial GitHub issue, the problem is wider and can be seen with other actions as well as you mentioned (mark task as) |
|
Thanks @pierrejeambrun and @matthieuauger for the detailed analysis. I was focused on the symptom and missed the underlying problem of the component identity being lost on each render. Using useMemo to stabilize the column definitions is a much cleaner and more scalable approach. |
|
@0lai0 did you manage to get this fixed? I see in your Pull Request that you still have old code and not only the useMemo, does it work with only useMemo? This bug is a little annoying on the daily basis it would be awesome to get it fixed |
|
Thank you @matthieuauger for the clarification, it seems I misunderstood the direction. The last commit didn't fully address the issue. I'll correct this soon. |
bbovenzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work adding memoizations
…ache#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh.
|
@bbovenzi do we have a backport for this ? |
… running (apache#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh.
|
Manual backport from a merge conflict: #58538 |
#58538) * [v3-1-test] Popup is getting automatically closed when there is a DAG running (#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh. * undo simple auth changes * Update simple_auth_manager_passwords.json
#58538) * [v3-1-test] Popup is getting automatically closed when there is a DAG running (#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh. * undo simple auth changes * Update simple_auth_manager_passwords.json
…ache#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh.
…ache#57568) * Popup is getting automatically closed when there is a DAG running * data refresh affects only the table rows, not the dialog boxes. * use useMemo to memorize columns * modify hope the dialog state will not be lost due to auto-refresh.
Popup is getting automatically closed when there is a DAG running.
Adding dialog detection to prevent auto-refresh from closing open modal/dialog components during data updates
Closes: #57341
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.