-
Notifications
You must be signed in to change notification settings - Fork 49.7k
Log Custom Reason for the Suspended Commit Track #34522
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
60ac750 to
a73fd1d
Compare
| rootContainer: Container, | ||
| ): null | string { | ||
| if (state.waitingForViewTransition) { | ||
| return 'Waiting for the previous Animation'; |
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.
If the previous animation finishes before the css and images do, then technically after that we were suspended on css. But that's a nuance that would require more complexity to show.
Technically if we were suspended on CSS first and then it finishes and then we were only suspended on images we should also show two tracks but that's unnecessary detail.
| } | ||
|
|
||
| export function getSuspendedCommitReason(state, rootContainer) { | ||
| return ''; |
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.
Isn't this supposed to return null?
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.
Originally I had this as required to return a string since we are always going to delay in this path so not providing a reason would be an error. However, I ended up just returning null in the DOM but maybe that should just throw instead.
a73fd1d to
bb6391d
Compare
Stacked on #34511. We currently log all Suspended Commit as "Suspended on Images or CSS" but it can really be other reasons too now. Like waiting on the previous View Transition. This allows the host config configure this reason. Now when one animation starts before another one finishes we log that as "Waiting for the previous Animation". <img width="592" height="257" alt="Screenshot 2025-09-17 at 11 53 45 PM" src="https://github.com/user-attachments/assets/817af8b5-37ae-46d8-bfd1-cd3fc637f3f3" /> DiffTrain build for [b204edd](b204edd)
Stacked on #34511. We currently log all Suspended Commit as "Suspended on Images or CSS" but it can really be other reasons too now. Like waiting on the previous View Transition. This allows the host config configure this reason. Now when one animation starts before another one finishes we log that as "Waiting for the previous Animation". <img width="592" height="257" alt="Screenshot 2025-09-17 at 11 53 45 PM" src="https://github.com/user-attachments/assets/817af8b5-37ae-46d8-bfd1-cd3fc637f3f3" /> DiffTrain build for [b204edd](b204edd)
Stacked on #34522. <img width="1025" height="200" alt="Screenshot 2025-09-19 at 6 37 28 PM" src="https://github.com/user-attachments/assets/f25900f6-6503-48b1-876d-bd6697a29c6f" /> We already cover the time between "Starting Animation" and "Remaining Effects" as "Animating". However, if the effects are forced then we can still be animating after that. This fills in that gap. This also fills in the gap if another render starts before the animation finishes on the same track. It'll mark the blank space between the previous render finishing and the next render starting as "Animating". This should correspond roughly to the native "Animations" track.
Stacked on #34522. <img width="1025" height="200" alt="Screenshot 2025-09-19 at 6 37 28 PM" src="https://github.com/user-attachments/assets/f25900f6-6503-48b1-876d-bd6697a29c6f" /> We already cover the time between "Starting Animation" and "Remaining Effects" as "Animating". However, if the effects are forced then we can still be animating after that. This fills in that gap. This also fills in the gap if another render starts before the animation finishes on the same track. It'll mark the blank space between the previous render finishing and the next render starting as "Animating". This should correspond roughly to the native "Animations" track. DiffTrain build for [b4fe1e6](b4fe1e6)
Stacked on #34522. <img width="1025" height="200" alt="Screenshot 2025-09-19 at 6 37 28 PM" src="https://github.com/user-attachments/assets/f25900f6-6503-48b1-876d-bd6697a29c6f" /> We already cover the time between "Starting Animation" and "Remaining Effects" as "Animating". However, if the effects are forced then we can still be animating after that. This fills in that gap. This also fills in the gap if another render starts before the animation finishes on the same track. It'll mark the blank space between the previous render finishing and the next render starting as "Animating". This should correspond roughly to the native "Animations" track. DiffTrain build for [b4fe1e6](b4fe1e6)
Stacked on #34511.
We currently log all Suspended Commit as "Suspended on Images or CSS" but it can really be other reasons too now. Like waiting on the previous View Transition. This allows the host config configure this reason.
Now when one animation starts before another one finishes we log that as "Waiting for the previous Animation".