#5963-Disappearing Messages Cut off

[MarlowBrown]

Contributor checklist

• My commits are rebased on the latest main branch
• My commits are in nice logical chunks
• My contribution is fully baked and is ready to be merged as is
• I have tested my contribution on these devices:

• iPhone 16, iOS 18.2

---

Description

This pull request fixes #5963 by addressing a potential race condition that could cause a label to be excessively compressed by edge insets.

To verify the effectiveness of this fix, I reproduce the bugs step to ensure that it does not happen anymore. The changes made in this pull request ensure that the label compression issue is consistently resolved.

Simulator.Screen.Recording.-.iPhone.16.-.2025-02-17.at.02.35.08.mp4

[sashaweiss-signal]

This fixes one specific case, but breaks the visual separation between clustered messages. I suspect the root cause is more along the lines of the clustering logic being wrong.

[sashaweiss-signal]

I think that removing this will lead to other UI issues. We intentionally add that top padding for those isFirstInCluster items, to visually separate those clusters.

[MarlowBrown]

Just for clarification for my sakes; a thread (TSThread) is the collection of all messages between individuals (Or single individual). A cluster is a subcollection of a thread consisting of consecutive messages from a single individual. So the purple would be the entire thread and the red would be clusters. Would that be correct?

I'm going to look more into the itemViewState clustering logic. Additionally, would it be possible to replace this clustering logic with a uniform inset for all component system messages?

[MarlowBrown]

Hey @sashaweiss-signal !

I've spent the past few days investigating the issue with the cell not displaying correctly. Initially, I thought it was due to a race condition with isFirstInCluster and isLastInCluster, causing the Label to have incorrect insets. However, I've uncovered a more significant problem that I think warrants attention.

After reviewing the item clustering logic, I found that it was producing the correct results. As you can see in the attached image, after the OutgoingMessage is deleted and new messages are reloaded and turned into CVItems by the CVItemModelBuilder, they have the correct ItemViewState.

Next, I examined the logic for turning CVItems into CVRenderItems. After creating a rootComponentView with the respective CVComponentView, I noticed that it received a measurement. I investigated the measurement logic and found that it was also producing the correct results. The attached image shows that the ManualStackView measurements are the same before and after the OutgoingMessage deletion.

Despite the correct measurements, I re-examined the Debug Views for the disappearing message view when the bug occurred and after a debounce event reloaded the views. I discovered that the "bubble" view (marked with a purple arrow) was incorrect. Although the itemViewState was correct, the view was not. The bottom disappearing message's bubble view had all corners rounded, whereas it should only have the bottom two corners rounded.

After the bug has happened and before the debounce update:

After the debounce update:

I set breakpoints when the bubble view was created and modified and found that when I reproduced the bug, the only call to configureForRendering was made for the top disappearing message (excluding the thread details message). I backtracked to the CVLoadCoordinator, then to the CVLoadCoordinatorDelegate in the ConversationViewController extension, and finally to the batch update block.

In the batch update block, I realized that the .update case was incorrect. If a new cell is added, the old index is always used instead of the new index, causing the cell to not update correctly. If a debounce event doesn't occur immediately after the update event, the cell won't be updated for 30 seconds. I believe this might be the cause of many problems. Attempting to use the new index update results in a crash, as updating and deleting the same indexPath is not supported.

To address this issue, I see a few options:

1. I leave it as is, as it's not a low-risk area, and let someone on the team handle it.
2. If you let me, I could Implement a BatchUpdateQueue to process batch updates in a queue-like fashion, separating update cell calls from other cell calls.

Another potential solution to consider for the future is utilizing UICollectionViewDiffableDataSource, which is a more modern approach to handling batch updates. It uses an identifier instead of an index path, allowing for more flexibility and avoiding issues like this bug. We're already using it in the Call functionality (CallDrawerSheet, CallLinkBulkApprovalSheet, CallListViewController) for table views. This approach could potentially simplify our batch update logic and make it more robust.

This bug has been a challenging journey, and I've become familiar with the process of loading messages, from the CVLoadCoordinator using a CVLoader to turn messages into CVItems, which are then turned into CVRenderItems and given their respective CVComponentViews. These are measured and turned into a LoadState, which is then converted into a CVRenderState and subsequently a CVUpdate. The CVUpdate is used with the CVLoadCoordinatorDelegate to update all cells in the ConversationViewController collection view. Finally, each cell is configured with its CVRenderItem, which lays out its view with its CVRootComponents and configures the cell root component.

Let me know how we should tackle this moving forward.

Cheers,
Talon

[sashaweiss-signal]

Hey @MarlowBrown,

As discussed over email, given the complexity of the underlying issues that you've identified, I think this is something we should leave alone for now. Rebuilding the ConversationViewController isn't something we're interested in taking on currently, and any changes to how it renders or loads is very high-risk and would need careful consideration, design, and review.

Thanks for looking into this.

[MarlowBrown]

Of course! This bug is extremely complex and will require a lot of resources to address, but as long as we are aware of it, that is a good first step. I will leave this open for now so it doesn't get lost in the closed PRs.

[sashaweiss-signal]

Thanks for looking into this. Since this isn't ultimately how we'll end up addressing this or related issues, I'm going to close this PR.