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

Attempt to fix issues with the notifications page state #1109

Closed

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Feb 2, 2024

Pull Request Description

Note

I am going to leave this as a draft for now until I've had a chance to daily drive it.

This PR is an attempt to fix an issue I've been experiencing lately. I haven't been able to reproduce it on the emulator (so no demo vid) but I see it a lot on my phone. Essentially, I'm working my way through notifications, navigating to each comment thread and navigating back to mark the reply as read, and occasionally a read message will reappear in the list after I navigate back from a post.

I believe this is happening because the InboxRepliesView is constructed with an initial set of replies, which comes from a query that can include read posts. I'm thinking a rebuild is reconstructing things, instead of relying on the Bloc after the initial build. I believe the fix is to rely entirely on the Bloc and never pass the initial list in. That way, the state management (like removing read replies from the list) should always be accurately represented in the UI. It's possible we could further clean up the InboxRepliesView, now that we have better state, but I made the minimal changes necessary.

Review without whitespace.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member Author

micahmo commented Feb 2, 2024

Ok, after running this branch on my physical device, the fix didn't work (although I still think it's a good change). Somehow, read messages are getting back in the state after being removed here.

replies.removeWhere(matchMarkedComment);

My next theory is that the NotificationsReplyPage is being rebuilt, which would recreate the the whole InboxBloc with the original set of replies. I'm going to try creating that Bloc once in the page widget and see if that helps.

BlocProvider.value(value: InboxBloc.withReplies(replies)),

@micahmo
Copy link
Member Author

micahmo commented Feb 5, 2024

Ok I'm going a bit crazy with this one, especially since I can consistently see the problem with my main account on my phone, but almost never with my test account on my emulator.

My next theory 😆 is that, since NotificationsReplyPage is stateless, maybe the act of "rebuilding" it also causes its constructor to be called again, which would seed it with the original set of replies. My next test will be to make the page stateful and see if that helps.

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.

1 participant