You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Jasperora (via GitHub)" <gi...@apache.org> on 2025/07/13 07:24:51 UTC
[PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Jasperora opened a new pull request, #53307:
URL: https://github.com/apache/airflow/pull/53307
## Why
Related issue: [#53085](https://github.com/apache/airflow/issues/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
https://github.com/user-attachments/assets/a86b0410-aa02-43b2-b498-c47633051d80
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #53307:
URL: https://github.com/apache/airflow/pull/53307
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "Jasperora (via GitHub)" <gi...@apache.org>.
Jasperora commented on code in PR #53307:
URL: https://github.com/apache/airflow/pull/53307#discussion_r2206185764
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Header.tsx:
##########
@@ -65,6 +65,10 @@ export const Header = ({
const [note, setNote] = useState<string | null>(taskInstance.note);
+ useEffect(() => {
+ setNote(taskInstance.note ?? "");
Review Comment:
Sorry for that. I'll try fixing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #53307:
URL: https://github.com/apache/airflow/pull/53307#discussion_r2205376868
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Header.tsx:
##########
@@ -65,6 +65,10 @@ export const Header = ({
const [note, setNote] = useState<string | null>(taskInstance.note);
+ useEffect(() => {
+ setNote(taskInstance.note ?? "");
Review Comment:
We should always try to avoid useEffects when possible, especially when they update the state.
Let's just update the state during `onConfirm` function call below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #53307:
URL: https://github.com/apache/airflow/pull/53307#discussion_r2205376868
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Header.tsx:
##########
@@ -65,6 +65,10 @@ export const Header = ({
const [note, setNote] = useState<string | null>(taskInstance.note);
+ useEffect(() => {
+ setNote(taskInstance.note ?? "");
Review Comment:
We should always try to avoid useEffects when possible, especially when they update the state. Because right now this can cause an infinite loop of updating the state.
Let's just update the state during `onConfirm` function call below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3079413022
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3075165303
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3144920341
### Backport failed to create: v3-0-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/16678647180'> Run details </a>
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>v3-0-test</td>
<td><a href="https://github.com/apache/airflow/commit/bb87ad5da725c530be06fc2bcf13c3c9d579496a"><img src='https://img.shields.io/badge/Commit-bb87ad5-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
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:
```bash
cherry_picker --continue
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3144937265
Manual backport https://github.com/apache/airflow/pull/54025
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "Jasperora (via GitHub)" <gi...@apache.org>.
Jasperora commented on code in PR #53307:
URL: https://github.com/apache/airflow/pull/53307#discussion_r2206673452
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Header.tsx:
##########
@@ -65,6 +65,10 @@ export const Header = ({
const [note, setNote] = useState<string | null>(taskInstance.note);
+ useEffect(() => {
+ setNote(taskInstance.note ?? "");
Review Comment:
Hi @bbovenzi, thanks for your suggestion! After some research, I understand the concern about overusing `useEffect`. However, since `note` is a local state, `useEffect` here is only for syncing it with `taskInstance.note` when external updates occur. It won't influence props. If we only update state in `onConfirm` below, the UI may show outdated data after a backend or cache update.
As far as I know, using `useEffect` to watch `taskInstance.note` is the most reliable way to keep local state in sync with changing props in React. If there’s a better approach, I’m happy to learn!
Please let me know if I’m missing something—thanks again for the feedback and discussion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "Jasperora (via GitHub)" <gi...@apache.org>.
Jasperora commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3124482882
Sorry for the late reply. I would like to confirm my understanding regarding the modal and state management.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "Jasperora (via GitHub)" <gi...@apache.org>.
Jasperora commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3131113295
Please see the demo and see if it satisfies the need. Thanks.
https://github.com/user-attachments/assets/639b6362-2bff-4568-94a6-692580d514a6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3096864925
> 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 https://github.com/apache/airflow/pull/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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3077839784
> 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. Unmounting will solve the problem there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3126968826
https://github.com/apache/airflow/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "Jasperora (via GitHub)" <gi...@apache.org>.
Jasperora commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3089540517
Hi @pierrejeambrun, I want to clarify that does
> 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` in `ClearTaskInstanceDialog` to modify note?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] auto-sync change of TaskInstance note without manual refresh [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #53307:
URL: https://github.com/apache/airflow/pull/53307#issuecomment-3098700921
> (@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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org