Fixes missing comments causing X more replies to show instead the missing comments

[MV-GH]

Before, the insertion of comments after a missing comment would go wrong. They would be inserted at the end of the tree

While the section that had the missing comments would have X more replies instead. Pressing this each time created more of those comments at the bottom of the tree. Once you scroll them into view it crashes as the tree can't have duplicate ids (we use the commentId as Id)

This ended up being far more complex than I thought it would be to fix. If there is a simpler way please let me know but this properly fixes it.

Fixes #1183

v2MXr0eqRn.mp4

[dessalines]

This even changes the building tree logic, ugh. I have no idea why we aren't having this problem with lemmy-ui, as it uses the same code. This is one where the complication its adding its just not worth it.

Because it is inserting twice the same comment because the read more replies button doesn't disappear

So then why don't we just hide / disable that more replies button after its been clicked once?

Or just randomly generate an id?

[MV-GH]

LemmyUI does have a problem, it infinitely loads when this happens

Example:
go to this comment and click load more, infinitely loads there because the comment insertion logic is faulty (doesn't account for this edge case where a parent can be missing)
https://programming.dev/comment/1988963

This change definitely needs to happen, (doesn't have to be in this manner though, but I don't see a simpler solution atm) The button isn't the problem. It adding the comments at the bottom because of the current logic because its missing comments. So its inserting it at the wrong nodes

 child?.let { cChild ->
            val parentId = getCommentParentId(cv.comment)
            parentId?.let { cParentId ->
                val parent = map[cParentId]

                // Necessary because blocked comment might not exist
                parent?.let { cParent ->
                    cParent.children?.add(cChild)
                }
            } ?: run {
                tree.add(cChild) // This is what makes it go wrong, comments which have missing parent, get added at end because of this
            }
        }

[MV-GH]

This problem, can be especially frequent (from what I heard) is when you start blocking users, as their comments won't be sent at all. Which can be in the middle of thread trees

[dessalines]

So maybe the getCommentParentId needs to also check the existing comments, to make sure that parent exists. Then it won't try to insert it. (Or it could toast an error).

This PR is related and should hopefully fix some of the issues with missing branches (at least when the comment is deleted or removed), but not all (at least with user blocks).

IMO this case should be handled in a much simpler way, but just checking to make sure the parent exists first.

[MV-GH]

I am not entirely following with the getCommentParentId changes, This util function just returns the id of the parent by checking the path and getting the one before the last.

That PR only solves one fraction of the problem. Not the problem that LemmyUI can't navigate nor correctly construct trees with missing comments in between them. (Neither can Jerboa but this solves that)

IMO this case should be handled in a much simpler way, but just checking to make sure the parent exists first.

We do that already and we insert them at the end. If there is no parent. But this causes different "trees" to mashup together look like they are part of the last comment tree but they aren't and also causes a crash for Jerboa because it cant handle duplicate comments

[dessalines]

This util function just returns the id of the parent by checking the path and getting the one before the last.

I'm recommending that we not just check the string, but also make sure the parent exists in the comment node list. The root cause of this issue is that its trying to construct a tree branch with a missing parent, which is impossible.

[MV-GH]

I'm recommending that we not just check the string, but also make sure the parent exists

We do that already. I marked above exactly where that happens.

The root cause of this issue is that its trying to construct a tree branch with a missing parent, which is impossible.

We can solve this by creating the missing comments nodes. Which is what this PR does.

[dessalines]

See this issue , and what I propose the best way to solve it is: to not have any missing comments in trees, and use properties like deleted, removed, creator_blocked on the CommentView to render them correctly, but still have the full tree available.

Faking missing comments isn't a good long-term solution for this problem, and its one that all the front ends are going to have to deal with, and they shouldn't have to deal with adding these super-complicated hacks for missing comments.

Also I haven't tested this, but there could be multiple levels of missing comments, not just a single level, which means you'd need to recursively fake them.

[MV-GH]

See LemmyNet/lemmy#3965 , and what I propose the best way to solve it is: to not have any missing comments in trees, and use properties like deleted, removed, creator_blocked on the CommentView to render them correctly, but still have the full tree available.

I'll respond there too, I do prefer this as we can show reasons for hidden comments (and more info that is missing now)

Also I haven't tested this, but there could be multiple levels of missing comments, not just a single level, which means you'd need to recursively fake them.

Yes that is correct, it can even have a missing top comments. And this solution/PR solves this too. See recCreateAndGenMissingCommentData Which recursively walks through the tree on insertion of a comment with a missing parent. To recreate the missing comment (and its potential missing parent, ...)

Faking missing comments isn't a good long-term solution for this problem, and its one that all the front ends are going to have to deal with, and they shouldn't have to deal with adding these super-complicated hacks for missing comments.

I do not consider this a "hack". It does increase the complexity of the tree creation logic, but most of the complexity is dealing with the types. I could avoid this by creating "fake" commentViews (instead of Either<MissingCommentNode, CommentVIew>) but I didn't want to do this as it feels more "hacky".

And yes, this indeed a problem all clients will have to deal with. (And I assume some already do, as I have not heard of any problems yet)

It is naive to assume that the server will always return complete trees. During the CSAM problematic period, admins were deleting comments straight from within the DB. How would the server solve these problems?

[MV-GH]

I think, i could change the Either<Missing, CommentView> to A sealed type if that is preferred. Would probably bring down type complexity a bit.

If it puts you more at ease I can write unit tests, to insure that the new comment tree creation logic works as intended.

[dessalines]

I could go line by line to try to decipher what these 3+ functions you've added are doing and how they fit together, because I can barely follow it. Several of them doing more loops than the two that are used to build a simple tree, which comes from simple tree-building pseudo-code that's proven reliable.

My order of preference from best to worst would be :

• Fix this issue on the back end.
• Don't build tree branches for missing comments, just ignore it.
• Make complicated tree logic for faking non-existing tree nodes like you've done here.

This is a very dangerous PR to merge IMO. At the very least please add a TODO with this PR link / commit so we can remove it when the back-end code is fixed.

[MV-GH]

This logic was wrong and caused the depth of the commentTree to be reversed.

Because this incorrectly assumes that the first comment has the lowest depth. (This is not always case such as when you use the parent_id due to a bug in 0.18.3) Thus causing the list to be generated with negative depth, This doesn't a problem (by sheer luck?) because the color handles this negative depth because it uses modulo and the calculateCommentOffset handles this correctly because it uses abs.

Now, It does it always correctly because it gets the depth of the root comment. (which is always the lowest)

see https://lemmy.one/api/v3/comment/list?max_depth=6&parent_id=3355587&sort=Hot&type_=All&auth=REDACTED

Before

After:

[MV-GH]

@dessalines I have removed one of the complex methods. Added more comments to the recursive one added tests for the buildCommentTree, simplified the generic commentNodeData holder to a Sealed type

I do believe this change is absolutely necessary. It can be removed once we no longer support a version of Lemmy that has this problem. But this won't be fixed 0.19. And when I ask users for their crash logs. This is the only crash that shows up. And it is very frequent (occurring daily).

[dessalines]

Very well. I'd thought lemmy v0.19 had this fixed tho. If it doesn't that needs to be addressed, because this affects all UIs.