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

[IMPROVE] Open rooms quicker #13417

Merged
merged 7 commits into from
Feb 13, 2019
Merged

[IMPROVE] Open rooms quicker #13417

merged 7 commits into from
Feb 13, 2019

Conversation

rodrigok
Copy link
Member

@rodrigok rodrigok commented Feb 8, 2019

Before

screen shot 2019-02-12 at 5 40 39 pm

After

screen shot 2019-02-12 at 5 35 59 pm

@rodrigok rodrigok added this to the 0.74.3 milestone Feb 8, 2019
@rodrigok rodrigok requested a review from ggazzo February 8, 2019 18:00
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-13417 February 8, 2019 18:00 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 8, 2019 18:49 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 11, 2019 14:26 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 11, 2019 16:23 Inactive
@ggazzo
Copy link
Member

ggazzo commented Feb 11, 2019

I made some tests and seems there are some bugs (mainly with scrolls)... We already made some solutions like that, and we never get the perfect solution =/

@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 11, 2019 19:11 Inactive
@rodrigok rodrigok force-pushed the improve-rooms-opening branch from 2d0f5a8 to b11d411 Compare February 11, 2019 20:07
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 11, 2019 20:07 Inactive
@rodrigok rodrigok force-pushed the improve-rooms-opening branch from b11d411 to 6170d8e Compare February 11, 2019 20:39
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 11, 2019 20:39 Inactive
@rodrigok
Copy link
Member Author

@ggazzo fiz passar nos testes

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

looks like message edits are not working.

sometimes the last message sent stays gray (for the sender only)

there is a huge delay between sending the message (hitting enter) and the message showing on the list.

@ggazzo
Copy link
Member

ggazzo commented Feb 12, 2019

hey @rodrigok its not just about the tests... please read that comment:

I made some tests and seems there are some bugs (mainly with scrolls)... We already made some solutions like that, and we never get the perfect solution =/

I meant: 'I made some test with YOUR branch and the behavior its not good, scrolls are flickering'.

and more we cant use the aproach using debounce/throttle if we have sequential code before wainting the render.

like: https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-ui-utils/client/lib/RoomHistoryManager.js#L96
here we do calcs base on scroll position after the messages get inserted. you can insert a lot of problems doing thinks like that.

@rodrigok rodrigok temporarily deployed to rocket-chat-pr-13417 February 12, 2019 18:11 Inactive
@ggazzo
Copy link
Member

ggazzo commented Feb 12, 2019

@sampaiodiego, its not a delay its self... its a "two debounced upserts", but the user feels like a delay =/ , if you keep sending messages with a rate < debounce time, you will never render the messages.

there is a huge delay between sending the message (hitting enter) and the message showing on the list.

@ggazzo
Copy link
Member

ggazzo commented Feb 12, 2019

@rodrigok if you pay attention here:

screenrecording20190212at4

after load more messages, the scroll jumps to the "bottom"�. the reason I have explained above.

@rodrigok
Copy link
Member Author

@ggazzo your explanation was wrong, the debounce has nothing to do with the scroll behavior or the delay rendering the message, both was caused by the full recalculation of the entire list caused by the usage of the ReactiveVar instead of the cursor. I moved back the the cursor cuz it wasn't the main improvement at the time.

The fail editing the message was caused by me using updated instead of changed in observeChanges.

I did some profilings and found the items that are most expensive being the find for permissions that happens several times per message render duo to the action buttons logic and the translate logic that execute finds agains the subscription collection (that may be big for certain users).

@ggazzo
Copy link
Member

ggazzo commented Feb 12, 2019

@rodrigok I gave up about discuss about use debounce or not (my explanation could not explain that specify bug, but prevent for many others), my final opinion (I dont think will change event with more chats): do not use debounce in some places, that case is one of them.

but I liked the memoize usage to avoid permissions recallcs (I think we should create a rocketchat helper/package to do that)

very smart the way you disabled the queries :)

I am testing now

@rodrigok rodrigok merged commit 098e560 into develop Feb 13, 2019
@rodrigok rodrigok deleted the improve-rooms-opening branch February 13, 2019 13:20
sampaiodiego pushed a commit that referenced this pull request Feb 14, 2019
* [IMPROVE] Open rooms quicker

* Close observer on room destroy

* Fix tests

* Back to cursor and improve permission query

* More improvements
@sampaiodiego sampaiodiego mentioned this pull request Feb 14, 2019
bhardwajaditya pushed a commit to bhardwajaditya/Rocket.Chat that referenced this pull request Mar 17, 2019
* [IMPROVE] Open rooms quicker

* Close observer on room destroy

* Fix tests

* Back to cursor and improve permission query

* More improvements
wreiske added a commit to wreiske/Rocket.Chat that referenced this pull request Apr 28, 2019
…nto ldap-admin-groups

* 'develop' of https://github.com/RocketChat/Rocket.Chat: (21 commits)
  Regression: Active room was not being marked (RocketChat#14276)
  Rename Cloud to Connectivity Services & split Apps in Apps and Marketplace (RocketChat#14211)
  LingoHub based on develop (RocketChat#14178)
  [IMPROVE] Replace livechat inquiry dialog with preview room (RocketChat#13986)
  Bump version to 0.74.3
  Room loading improvements (RocketChat#13471)
  [FIX] Invalid condition on getting next livechat agent over REST API endpoint (RocketChat#13360)
  [IMPROVE] Open rooms quicker (RocketChat#13417)
  [FIX] "Test Desktop Notifications" not triggering a notification (RocketChat#13457)
  [FIX] Translated and incorrect i18n variables (RocketChat#13463)
  Regression: Remove console.log on email translations (RocketChat#13456)
  [FIX] Properly escape custom emoji names for pattern matching (RocketChat#13408)
  [FIX] Not translated emails (RocketChat#13452)
  Added missing package dependency (RocketChat#13437)
  Update Russian localization (RocketChat#13244)
  [IMPROVE] Allow configure Prometheus port per process via Env Var (RocketChat#13436)
  [IMPROVE] Add API option "permissionsRequired" (RocketChat#13430)
  [FIX] Several Problems on HipChat Importer (RocketChat#13336)
  Add the missing uniqueId to the push notifications (RocketChat#13423)
  [FIX] Notify private settings changes even on public settings changed (RocketChat#13369)
  ...
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.

4 participants