-
Notifications
You must be signed in to change notification settings - Fork 158
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
fix overfetch channel actions #1748
Conversation
const channelId = msg.data.channel_id; | ||
const isAlreadyViewed = Boolean(msg.data.prev_viewed_at); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
// 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]) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- add the new field in the websocket event and use it to decide if we have to trigger welcome message
- 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)
There was a problem hiding this comment.
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.
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. 🤔 |
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). |
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:
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 updatedGated by experimental feature flagUnit tests updated