Fix/blur shrink ondelete

[zukilover]

Fixes #2527

Changed animation name from very-fast-fade to very-fast-morph as it also includes other transformation and filter as well, i.e. scale3d and blur.

This PR depends on #2581 since it needs the newly added CSSTransition.

[raineorshine]

Thanks! Let's wait for #2581 before reviewing since merge conflicts are expected. Please rebase on upstream main and resolve merge conflicts once ready.

[zukilover]

@raineorshine I'm still trying to figure out how to capture the thought data while it's being deleted. The short-circuit here prevents animating during the delete process, causing it to appear empty before the animation finishes:

em/src/components/VirtualThought.tsx

Lines 201 to 203 in be45dec

	// Short circuit if thought has already been removed.
	// This can occur in a re-render even when thought is defined in the parent component.
	if (!thought) return <span>deleted</span>

[raineorshine]

Removed the short circuit in 0d71558. Hope that helps!

[zukilover]

Since the Subthought component currently short-circuits and returns null when the thought is undefined, I've implemented an alternative approach by caching the DOM of the Subthought before it gets deleted. This allows the cached DOM to remain visible during the 80ms nodeDissolve transition, enabling a smooth animation effect. By caching the DOM, we ensure that the transition is visible without triggering any store-related logic or reactivity, keeping the animation independent of the state management process.

[trevinhofmann]

The shrink/blur/fade animation for deletion generally seems to work as expected, although it's a bit difficult to notice at full speed:

Screen.Recording.2024-12-24.at.1.53.00.AM.mov

Slowing the animation down frame-by-frame confirms that the animations are working, except for one issue. At the beginning of the animation, the thought is empty for ~1 frame:

Screen.Recording.2024-12-24.at.1.51.14.AM.mov

---

#2527 requests that this animation apply for both deletions and collapsing of context. Do you plan to add the animations for collapsing context in this PR as well?

[trevinhofmann]

Thank you very much, @zukilover!

The animation is now being applied correctly to collapsing context 👍

These changes also resolved the initial "blink" before the animation starts. However, I am now noticing that the other thoughts do not animate up until the blur/shrink/fade animation completes:

Screen.Recording.2024-12-27.at.6.50.41.PM.mov

I also noticed that the thoughts do not get cleaned up / removed from the DOM after they've been deleted. Here are three deleted thoughts still in the DOM, which should probably be removed once the animation completes:

[zukilover]

Hi @trevinhofmann I don't see how it's possible to animate the next thought up before the previous one has not been completely deleted because transitionGroupsProps.in —which triggers the animation— will be effective when treeThoughtsPositioned changes. If we compare it to main branch, next node will move up when nodeFadeOut is completed too. Let me know if this makes sense.

Also the DOM not being removed also happens in the main branch, I wonder if this should be treated a separate issue.

[raineorshine]

When a thought is deleted, treeThoughtsPositioned should immediately be updated and bump the y positions of subsequent thoughts, triggering the translate animation. You can see the other thoughts moving up even as b is fading out in this video from main. This was discussed and refined in #2581 (review) (4):

Screen.Recording.2024-12-31.at.10.02.48.AM.mov

When I was setting up the video I did notice that there were some cases where the fade out animation did not occur at all, which would seem to be either a regression or bug in #2581. The above video shows the expected behavior with the fade out. The intention of #2527 is to replace the fade out animation with fade out + shrink + scale without affecting the timing or other animations like other thoughts translating up.

@trevinhofmann @zukilover Could you both confirm that the fade out animation is working for you on delete in main? If #2581 is buggy it could make solving this issue more difficult. I recommend deleting a few thoughts in a row, from the beginning, middle, and end of a list to test it out.

Also the DOM not being removed also happens in the main branch, I wonder if this should be treated a separate issue.

Thanks for confirming that. Created #2751.

[trevinhofmann]

@trevinhofmann @zukilover Could you both confirm that the fade out animation is working for you on delete in main? If #2581 is buggy it could make solving this issue more difficult. I recommend deleting a few thoughts in a row, from the beginning, middle, and end of a list to test it out.

Testing main (42b1ef5), I see:
A) ✅ Thoughts fade when collapsing context
B) ❌ Thoughts do not fade when deleting
C) ✅ Subsequent thoughts immediately shift up when collapsing context
D) ✅ Subsequent thoughts immediately shift up when deleting

Testing fix/blur-shrink-ondelete (2a51742), I see:
A) ✅ Thoughts fade/blur/shrink when collapsing context
B) ✅ Thoughts fade/blur/shrink when deleting
C) ✅ Subsequent thoughts immediately shift up when collapsing context
D) ✅ Subsequent thoughts immediately shift up when deleting

I believe #2581 and #2526 were specifically for collapsing context, and I'm not seeing issues there. Deleted thoughts are not fading on main, but appear to be fading/blurring/shrinking correctly now in this PR.

Hi @trevinhofmann I don't see how it's possible to animate the next thought up before the previous one has not been completely deleted because transitionGroupsProps.in —which triggers the animation— will be effective when treeThoughtsPositioned changes. If we compare it to main branch, next node will move up when nodeFadeOut is completed too. Let me know if this makes sense.

I'm not sure if that makes sense to me. The (C) and (D) cases work as expected for me on main, but were not working as expected in this PR when I made this comment.

That said, (C) and (D) appear to have been fixed in the current version of this PR 🎉. Have you made any recent changes that were expected to resolve this?

Also the DOM not being removed also happens in the main branch, I wonder if this should be treated a separate issue.

Good catch! Apologies, and I agree with you and @raineorshine that this should be a separate issue (#2751) in that case.

[trevinhofmann]

Marking this as approved from my end. With the current changes, the code still looks fine to me, and the fade/blur/shrink animations (and shifting up of subsequent thoughts) are now working as expected for me on every deletion and collapsing context. The animation is quite fast, so you may need to step frame-by-frame to confirm:

Screen.Recording.2024-12-31.at.8.14.47.PM.mov

---

@raineorshine -- Can you please retest the cases where the fade was not working on main to ensure that the fade/blur/shrink works as expected for you with this PR? I haven't been able to reproduce the missing animation here.

@zukilover -- Can you please explain what changes were made since the last review? Due to the rebase (which is fine), I can't see what exactly has changed since then. It would be nice to know the changes because your comment indicated that you weren't able to fix the shifting of subsequent thoughts, but the latest changes seem to have fixed it.

Thanks!

[zukilover]

@trevinhofmann, the only change in the latest rebase was the removal of the type assertion here:

em/src/components/Subthought.tsx

Lines 63 to 64 in 1402dca

	const cachedHTMLRef = useRef<string | null>(null)
	const thought = useSelector(state => getThoughtById(state, head(simplePath)) as ThoughtType | undefined, shallowEqual)

Additionally, I assume the anomaly you pointed out here:
#2596 (review) was caused by slowing down the duration (likely layoutNodeAnimation), which made the transition appear out-of-sync.

Here's the latest result from this PR. Please confirm if it works as expected:

delete-animation.mp4

[raineorshine]

@trevinhofmann Thanks for the thorough testing. I can also confirm that the fade/shrink/blur is working as expected now. The cases where there is no fade/shrink/blur are explainable:

• The animation happens so fast that sometimes my screen recorder doesn't even capture it. Upon closer review I can see the small deleted thought very faintly in one of the frames which confirms that it is animating.
• Deleting the very last thought in the thoughtspace does not trigger the fade/shrink blur as it causes the "Hit the Enter key to add a new thought" message to appear. This will never happen in practice once the user has at least one thought, so we can ignore this case.

I believe #2581 and #2526 were specifically for collapsing context, and I'm not seeing issues there. Deleted thoughts are not fading on main, but appear to be fading/blurring/shrinking correctly now in this PR.

#2526 was intended to create a fade out for all unmounted thoughts, not just collapse. This PR overrides that behavior for delete.

[raineorshine]

Thanks for making the requested changes! The new approach is more robust and type safe.

Looking over the implementation again, I have some reservations about the way the DOM is utilized in this performance critical part of the application. document.createElement, document.querySelectorAll, and element.classList.remove could all be avoided with a simple .replace. I have to say that I would prefer that approach. It's just not worth the risk of impacting the performance here.

[zukilover]

Sure, @raineorshine. I've refactored the function and use a regular expression for string replacement, which can be faster and more efficient than iterating through an array of CSS classes.

For most practical scenarios, the performance difference is likely to be negligible.
Thanks.

[raineorshine]

Thank you!

Yes, I totally agree. You'll have to trust me that this is one of those exceptions! Performance of thought rendering is critical in this application, and DOM access has a cost. There are already noticeable performance issues, so we don't want to inadvertently make it worse.

[raineorshine]

Looks good, thanks!

I did some minor refactoring and renaming in 526c7e5. I recommend taking a quick look over the changes.

[raineorshine]

I would suggest a more specific name for this hook, e.g. useCachedThoughtHtml, since it has several thought-specific pieces of functionality.

We can also add to the JSDOC to explain a little more about what it does and where it's useful.

[raineorshine]

The commend needs a slight tweak. It captures the HTML not just when the thought is first rendered, but whenever the thought changes.

[raineorshine]

Now that this is just a single .replace, it doesn't need to be wrapped in a function.