auto-sync change of TaskInstance note without manual refresh

[Jasperora]

Why

closes: #53085
In TaskInstance/Header.tsx, the note state is not refreshed when the TI is refreshed from new cache value.
It needs manual refresh to solve this mismatch.

What

Added a useEffect that listens for changes to taskInstance.note and updates the local note state accordingly.
No manual refresh is needed.

Demo

ti-note-demo.mov

---

^ 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.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[pierrejeambrun]

Yes we should try to avoid useEffect as much as possible, not sure how to get away on this one. (Maybe we need to keep a state to the original value coming from the prop, and only re-sync them when we observe a diff between the prop and the original value, but that would be fairly close to a useEffect in practice).

Other things that I can think of are worse than a useEffect (using a forwarded ref for instance).

[bbovenzi]

Honestly, I think notes need to be reworked.

The modal needs two clicks, one to remove focus, another to actually confirm. The textarea in the header also moves too much page content around. We should probably just move it back to only edit in the modal and unmount the modal every time its closed

[pierrejeambrun]

The modal needs two clicks, one to remove focus, another to actually confirm. The textarea in the header also moves too much page content around. We should probably just move it back to only edit in the modal and unmount the modal every time its closed

That would do it, I'm also in favor of this and reverting back to the modal edit only. Unounting will solve the issue, I tried to go that way but it's less obvious how and when we can unmount this accordion (modal will be fairly simple though).

So basically @Jasperora we need to go back to what we had before:

• Remove the whole accordion with the note edit / display in the Details tab.
• All "Add a note" / "Edit note" should go through the dedicated button that opens up the modal.
• Unmounting that modal in between edits should solve the problem of the content not refreshing.

You can take a look at the PR that introduced that accordion to revert back the behavior #51764

[jscheffl]

The modal needs two clicks, one to remove focus, another to actually confirm. The textarea in the header also moves too much page content around. We should probably just move it back to only edit in the modal and unmount the modal every time its closed

That would do it, I'm also in favor of this and reverting back to the modal edit only. Unounting will solve the issue, I tried to go that way but it's less obvious how and when we can unmount this accordion (modal will be fairly simple though).

So basically @Jasperora we need to go back to what we had before:

* Remove the whole accordion with the note edit / display in the Details tab.

* All "Add a note" / "Edit note" should go through the dedicated button that opens up the modal.

* Unmounting that modal in between edits should solve the problem of the content not refreshing.

You can take a look at the PR that introduced that accordion to revert back the behavior #51764

I understand that the note refresh is not good and also understand that inline editing adds complexity. I actually did not want to make it more complex... main target of #51764 was to have the note present when a user clicks on a task instance.

For our (and I assume many other) purposes the note is to inform users about "something" operational or having a high level summary - not forced to be used... but can be a guide. If I need to click a button to see the note then the whole purpose of the note is void... then I can "hide" my details somewhere in the logs as well.

TLDR: I'd vote for keeping (at least the option) to display the note immediately w/o modal when a task instance is clicked on. But reverting back #51764 to the previous state would be not good in my view.

[Jasperora]

Hi @pierrejeambrun, I’d like to confirm my understanding regarding the note editing.

Does the statement

All "Add a note" / "Edit note" should go through the dedicated button that opens up the modal.

mean that we use EditableMarkdownButton as the only button to modify note, and do not allow ActionAccordion within ClearTaskInstanceDialog to modify note?

[Lee-W]

Looks good from a translation perspective

[pierrejeambrun]

mean that we use EditableMarkdownButton as the only button to modify note, and do not allow ActionAccordion within ClearTaskInstanceDialog to modify note?

Not exactly. We should remove EditableMarkdownButton from Run/Header.tsx and TaskInstance/Headers.tsx and force unmounting of modals so the initial state is recomputed when the modals (clear TI or edit a note) are mounted again.

The issue is with the inlined note in the task details. That should be removed because it will be hard to 'unmount' that and force a refresh, unless we add an ugly useEffect there, but this piece was actually bringing other problems, so I'm for removing it.

[pierrejeambrun]

TLDR: I'd vote for keeping (at least the option) to display the note immediately w/o modal when a task instance is clicked on. But reverting back #51764 to the previous state would be not good in my view.

Yeah not ideal, we let that on main for a while with the idea to improve it and make it better but I couldn't find time to do that, and a month later it's still causing problem. We should revert and merge it back when it's properly implemented and cover all cases, it's too messy at the moment. (@jscheffl unless you have time to spend on it).

Maybe put that into it's own tab ? Or add a marker to the StatusBadge as we had in AF2 to indicate that there is a note there ?

[jscheffl]

(@jscheffl unless you have time to spend on it).

I attempted to propose alternatives... but before adding more that are rejected we shoudl agree on a solution space. Mhm not sure and lost overview about the options. Shall we maybe list the options with sketches in Confluence and the vote / discuss on these? I could propose contributing to it... next week (as I am on vacation tomorrow - Sunday).

[pierrejeambrun]

Yes I think it would be a great idea to brainstorm a little bit about that note visibility use case, I think the note in the header was not a bad idea but it needs some more thoughts to be fully working.

I would be glad to review any design, it doesn't have to be a formal confluence page, a github issue would work too.

While we do that i'm more concern about:

• not delivering a non working feature for 3.1.0 or something that has not the correct UX.
• not having it delay other bug fixes like this one

Since I cannot commit to this considering my other priorities, and it appears that no one else could during the last month this was on main. I suggest we revert so it leaves us all the time in the world to come up with a better approach without pressure (for 3.1.0 or for 3.1.0+), unblocking this bug fix.

Here is the partial revert for now. In this PR to solve the issue we will only need to be sure that we unmount the different modals with EditableMarkdownArea.
#53635

[Jasperora]

Sorry for the late reply. I would like to confirm my understanding regarding the modal and state management.

In this PR, we want to remove the EditableMarkdownArea in both Run/Header.tsx and TaskInstance/Header.tsx, but retain EditableMarkdownButton in these two files to allow users to open up the modal through the button. And we want to unmount the modal in between edits to make it get the latest value each time it's opened.

Currently, the mdContent prop for EditableMarkdownButton is derived from the note state in either Run/Header.tsx or TaskInstance/Header.tsx. Unmounting the modal alone does not reset or update the note state in the parent component, so even if the modal is unmounted, mdContent may still reflect an outdated value if note was changed elsewhere.

To ensure that the modal always displays the most current content, it seems necessary to use an onOpen function to set the note state each time the modal is opened. This approach would guarantee that the initial state is refreshed appropriately.

Please let me know if my understanding is correct or not. Thank you again for your guidance.

[pierrejeambrun]

#53635 has been merged, you can rebase your pull request and focus your effort on the modal. I thought that unmounting would do the trick, maybe it doens't and an onOpen function to sync state can do it yes.

[Jasperora]

Please see the demo and see if it satisfies the need. Thanks.

auto-sync-demo.mov

[pierrejeambrun]

That's good enough for now, but later on we might want to follow the same pattern as ClearRunButton and MarkRunAsButton. So EditableMarkdownButton state management and definition is inside the component itself, in the 'dialog` part, which is unmounted when the modal is closed.

Similarly to ClaerRunButton and MarkRunAsButton that content would be refreshed without further need.

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker bb87ad5 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Manual backport #54025