Skip to content
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

Handle deferred comment loading issue #624

Merged
merged 3 commits into from
Aug 15, 2023

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Aug 11, 2023

Pull Request Description

I've been having issues with deferred comments not loading. I found the same issue with the same comments on the web, so it's not strictly a Thunder problem, but I wanted to dig in anyway. It turns out that we occasionally get comments where the none of the children are direct descendants of the parent. For example, the parent path may be 0.10 and the child with ID 30 may be 0.10.20.30. I'm not sure why it happens (maybe comment 20 got deleted?), but I thought it might be nice to handle this case.

Note that since we do actually get the child comments in the response, we could go a step further and insert placeholders for the missing comments so the results are still displayed. But since this seems like a backend issue (which the web client also fails to handle), I think at least showing a nice error message is an improvement.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Before (web and app)

chrome_32G3vtemst.mp4
qemu-system-x86_64_uyNhnxDu9T.mp4

After

qemu-system-x86_64_4CNO2okeG7.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable? -- No, none of these error messages are localized yet.
  • Did you add semanticLabels where applicable for accessibility? -- N/A

@machinaeZER0
Copy link
Collaborator

Is "unable to load direct children" our language, or Lemmy's? If possible I might change it to something simpler, like "unable to load comments". "Direct children" feels odd to me I think, even if that is the technical verbiage

@micahmo
Copy link
Member Author

micahmo commented Aug 13, 2023

It's our language (you can see the code here: https://github.com/thunder-app/thunder/pull/624/files#diff-d652fa040b3a56cfa0e6ef2042cc3a8fdd54216b6251776edbac9bad9fc52559R293). The problem here is moreso that we're directly showing the error (Exception) to the user. I think eventually we'll match all errors (from Lemmy or us) to a more human-friendly description.

For now, I was hoping the raw error would reference "direct child" because that's the very specific error that we're handling here, and if people report it, we'll know exactly what going on.

@machinaeZER0
Copy link
Collaborator

That's fair, helping keep error reports clear/specific is a good goal!

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hjiangsu hjiangsu merged commit 16a23d1 into thunder-app:develop Aug 15, 2023
1 check passed
@micahmo micahmo deleted the fix/handle-loading-error branch August 15, 2023 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants