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

fix overfetch channel actions #1748

Closed

Conversation

trilopin
Copy link
Contributor

@trilopin trilopin commented Jan 19, 2023

Summary

Two different actions on the channelactions management to reduce the number of calls on every channel switch. There are two kind of fetch:

/plugins/playbooks/api/v0/actions/channels/

Make channel actions modal fetch action ONLY when is shown, not when it's rendered but hidden. Before this change, channel actions were fetched on every channel switch.

/plugins/playbooks/api/v0/actions/channels/?trigger_type=new_member_joins

Several actions done here:

  • kept hasviewedchannel redux reducer/actions/seelctors to guarantee backwards compatibility with old servers
  • added prev_viewed_at to websocket data at mattermost-server
  • use prev_viewed_at to trigger just the first time

Before
https://user-images.githubusercontent.com/4096774/213535360-fda5437a-67b0-416d-bbfc-9f216c37fd81.mp4

After
https://user-images.githubusercontent.com/4096774/213535329-e6caa3d5-71d9-42dd-a531-17601581a656.mp4

This PR relies on some small change on mattermost-server. If the mattermost-server don't send that new field, we will work as we previously did (just prevent fetch after viewing channels once per session)

Ticket Link

Fixes #1731

Checklist

  • Telemetry updated
  • Gated by experimental feature flag
  • Unit tests updated

@trilopin trilopin added 2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Jan 19, 2023
const channelId = msg.data.channel_id;
const isAlreadyViewed = Boolean(msg.data.prev_viewed_at);
Copy link
Member

Choose a reason for hiding this comment

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

nit: Could we test msg.data.prev_viewed_at > 0 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be undefined if the server is not updated,

if it's undefined, we'll still use the old flow.

I can do msg.data.prev_viewed_at !== undefined && msg.data.prev_viewed_at > 0, if it feels more explicit

Comment on lines +208 to +210
// if the user has already viewed the channel, there's no need to fetch actions again
// prev_viewed_at is not coming for server 7.7 and below, so we additionally check redux
if (isAlreadyViewed || hasViewedByChannelID(getState())[channelId]) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: if we have this information in Redux, why do we need the extra metadata in the websocket event? Is there a chance the websocket data is "stale" relative to Redux?

Copy link
Contributor Author

@trilopin trilopin Jan 20, 2023

Choose a reason for hiding this comment

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

The following details are relevant for new_member fetch, not the modal one. that's another different problem solved with the modal show check.

I keep redux until we want to force server update to ease backward compatibility. After server version force we can remove all code related to viewedchannel (action/reducer/selector/db table/store layer).

The way it works today, it fetch actions on channel switch at least once per session (until redux is filled with the data).

We could solve the overfetch problem in two different ways:

  1. add the new field in the websocket event and use it to decide if we have to trigger welcome message
  2. keep the redux store at webapp, the ir_viewedchannel table, and the store layer at the server, and add an initial complete fetch of all my ir_viewedchannel when the plugin starts.

Option 1 seems more scalable to me, since we use a new tiny piece of info in the moment it's relevant, avoiding the storage/management of data that increases with the platform usage.

Just for the price of sending a timestamp (or a boolean if we want to simplify it more) we remove a lot of complexity and we avoid creating a new one (complete fetch on plugin start)

Copy link
Member

Choose a reason for hiding this comment

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

This all makes sense! Excited about the future simplification once we can rely on the presence of this field.

@lieut-data lieut-data added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jan 23, 2023
@trilopin
Copy link
Contributor Author

I realized that the welcome message won't be sent if the first view of a channel is done on the mobile app (and the user is not connected at the time in desktop/browser).

In the previous approach, mobile was also ignored, but at least the message was sent in the first desktop view.

🤔

@trilopin
Copy link
Contributor Author

Closed.

The modal one was already extracted to a different (merged) pr.

I wrote a proposal with different alternatives, and after some discussions I believe that would make sense to solve it 100% on server-side (hook or mpa).

@trilopin trilopin closed this Feb 13, 2023
@mattermost-build mattermost-build removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Feb 13, 2023
@trilopin trilopin deleted the gh-1731-avoid-unnecesary-fetch-channelactions-bis branch February 13, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

channel actions fetched twice on channel switch
4 participants