ProximaEPFL / proxima

A social app that encourages users to visit places in order to see posts.
8 stars 1 forks source link

Update comment count when adding a comment #349

Closed JoachimFavre closed 5 months ago

JoachimFavre commented 5 months ago

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:

gruvw commented 5 months ago

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 commented 5 months ago

Well, your bug is very strange. It appears the state = const AsyncValue.loading(); I added in 810bf0977a466278e0921c81264e6ec3baea86e8 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 e91ddb5e432776573b49303de529196f1cb95572. 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.

gruvw commented 5 months ago

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.