Bug: reaction and reaction-received fail to load the next page

Bug
1

In reaction/reaction-received page on activity/notification, there exists a bug.

When user trying to scroll down and load the next page, there will be the same items added to the list, causing duplication.

The issue stems from a misalignment between the frontend and the backend. The id defined in the frontend is post_id while the backend requires reaction_id.

A draft PR is here, please review it when convenient.

https://github.com/discourse/discourse/pull/36451

3 Likes
2

Thanks for the draft PR, it does seem like pagination for reactions is inaccurate now and seems to repeat.

I am not sure if the correct fix is to set the post_id. It looks like pagination uses before_reaction_user_id (see here). Potentially even removing line 27 (the PR) would help since it’s likely set in the flattened reaction above.

1 Like
3

Thanks for your reply.
I’ll inspect it tomorrow and see if something will fix that.

1 Like
4

Cool, thank you!

To be clear I think the before_reaction_user_id should be the id from discourse_reactions_reaction_users - I think that variable name is somewhat confusing. Please feel free to clarify anything here or in chat.

1 Like
6

I’ve investigated and found why the rspec failed.

The original spec is asserting a component with property expect(page).to have_css(".user-stream-item [data-post-id='#{post_1.id}']").

However, the data-post-id was changed to the reaction_user_id in the last commit, this caused a mismatch, leading to the failure.

The original thought is to change id to fit the api need, however, I missed the fact that the id is also used in PostList as post_id, like follows:

<PostList
    @posts={{@model}}
    @fetchMorePosts={{@controller.loadMore}}
    @emptyText={{i18n "notifications.empty"}}
    @additionalItemClasses="user-stream-item"
    @showUserInfo={{false}}
    class="user-stream"
  >

And the id used in it is mapped in post.gjs component

data-post-id={{@post.id}}
data-topic-id={{@post.topicId}}
data-user-id={{@post.user_id}}

So, the original id behavior shouldn’t be changed, as this may cause severe mismatches in the postList, that directly leads to the failure of rspec yesterday.

As a workaround, there might be another way to solve this:

Adding a new field reaction_user_id: reaction.id in flattenForPostList, then alter the return array.length ? array[array.length - 1].id : null; in #getLastIdFrom(array) func to return array.length ? array[array.length - 1].reaction_user_id.

TL:DR;

The reaction_id, post_id, and now the reaction_user_id are totally different ones, but the id used in the postList component needs to be post_id. While the id needed in fetching the next page should be reaction_user_id, that is extremely perplexing.

@nat

7

Thanks for the investigation and PR @small-lovely-cat :+1:

I’ve added a spec to the PR and will get it merged.

2 Likes
8

This topic was automatically closed after 19 hours. New replies are no longer allowed.