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

Update comment count when adding a comment #349

Merged
merged 24 commits into from
May 27, 2024
Merged

Conversation

JoachimFavre
Copy link
Collaborator

@JoachimFavre JoachimFavre commented May 25, 2024

Fixes #316.

This PR adds a PostCommentCountViewModel, which stores a post number of comment (in a similar fashion to PostVotesViewModel). It is refreshed by doing a fetch to the database when a comment is deleted, and refreshed efficiently when the comment list is refreshed on the post page.

This is then tested by taking advantage of the great work @Aderfish made for end-to-end tests.

Happy reviewing! :)

Acceptance criteria:

  • When commenting on the post page and going back to feed, the number of comments on post card should be updated

@JoachimFavre JoachimFavre self-assigned this May 25, 2024
@JoachimFavre JoachimFavre changed the title Update comment count on nvaigation Update comment count on navigation May 25, 2024
@JoachimFavre JoachimFavre marked this pull request as ready for review May 25, 2024 18:28
@JoachimFavre JoachimFavre requested a review from yoannLafore May 25, 2024 18:50
@JoachimFavre JoachimFavre changed the title Update comment count on navigation Update comment count when adding a comment May 25, 2024
@JoachimFavre JoachimFavre requested a review from Aderfish May 25, 2024 19:33
@CHOOSEIT CHOOSEIT requested review from gruvw and removed request for yoannLafore May 25, 2024 19:54
Copy link
Collaborator

@gruvw gruvw left a comment

Choose a reason for hiding this comment

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

Good code with extensive documentation. Left a few remarks, but it should be ready to merge soon.

lib/viewmodels/comments_view_model.dart Outdated Show resolved Hide resolved
test/end2end/app_actions.dart Show resolved Hide resolved
@gruvw
Copy link
Collaborator

gruvw commented May 26, 2024

I now have an error thrown to the Flutter console when deleting a comment from the user profile page:

E/flutter (17447): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Bad state: Future already completed
E/flutter (17447): #0      _AsyncCompleter.complete (dart:async/future_impl.dart:43:31)
E/flutter (17447): #1      FutureHandlerProviderElementMixin.onData (package:riverpod/src/async_notifier/base.dart:282:17)
E/flutter (17447): #2      AsyncData.map (package:riverpod/src/common.dart:345:16)
E/flutter (17447): #3      FutureHandlerProviderElementMixin.state= (package:riverpod/src/async_notifier/base.dart:205:14)
E/flutter (17447): #4      AsyncNotifierBase.state= (package:riverpod/src/async_notifier.dart:57:14)
E/flutter (17447): #5      PostCommentCountViewModel.refresh (package:proxima/viewmodels/post_comment_count_view_model.dart:23:5)
E/flutter (17447): <asynchronous suspension>

Note: this bug was not present on ef39f27.

@JoachimFavre
Copy link
Collaborator Author

Well, your bug is very strange. It appears the state = const AsyncValue.loading(); I added in 810bf09 causes this issue. If I remove it, the error message disappears.
After some more digging in riverpod's GitHub issues, I think this is because the provider gets unmounted in the middle of its refresh, causing an issue since we try to modify the internal state right after. I therefore made this provider not auto-dispose in e91ddb5.
However, I'm not entirely sure neither my understanding nor my fix are correct, since I'm not sure it explains why removing the state = const AsyncValue.loading(); would change anything, nor the exception name Bad state: Future already completed. @gruvw you have more experience than me in riverpod, I'd be very curious to hear your opinion about this.

Copy link
Collaborator

@Aderfish Aderfish left a comment

Choose a reason for hiding this comment

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

Clean code, I just left a small suggestion. I also would like to understand more about this weird bug and its apparent fix.

Apart from that LGTM.

test/end2end/app_actions.dart Outdated Show resolved Hide resolved
@gruvw
Copy link
Collaborator

gruvw commented May 27, 2024

I'd be very curious to hear your opinion about this.

I never encountered that bug and I don't really have time to dig too much into it this week.
As this provider doesn't really need to be AutoDispose I think we'll keep this solution for the moment.

If you think this should be investigated further, please create a new issue to the project's backlog.

Copy link
Collaborator

@gruvw gruvw left a comment

Choose a reason for hiding this comment

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

Thank you for your changes and the isolated testing. LGTM

@JoachimFavre JoachimFavre merged commit ef14a13 into main May 27, 2024
3 checks passed
@JoachimFavre JoachimFavre deleted the update-comment-count branch May 27, 2024 18:15
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.

Refresh comment count on navigation back
3 participants