One weird edge case was deleting/adding user to chat/thread when other user had overlay displayed.
Normally overlay is rerendered with newly fetched fresh user list, but it may cause weird shifts in chosen button.
We could solve it by using some extra state to save currently chosen button and then going back to it after list of users change, but I decided to go with simpler solution: when overlay is displayed list of users is frozen until it disappears.
It may result in tries to @ users that were deleted from channel, but it doesn't seem like a huge issue.
In future we may react to it by displaying modal asking if we want to sent invite to such user (or any user that we tried to @, but who was not in out channel).
Details
Details
Added and deleted users using another account and incognito browser.
Freezing worked fine.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Spotted uncaught bug while testing later on - it caused to freeze users list in wrong situations. Moved frozen users to state in context. It works now.
| web/input/input-state.js | ||
|---|---|---|
| 39 ↗ | (On Diff #20514) | I don't think it is a good idea that input state knows about internal implementation of a tooltip. We shouldn't keep frozenThreadMembers here. When we show a typeahead, it should save the list (probably in state) and update it only when it becomes invisible. |
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 598–610 ↗ | (On Diff #20514) | Is this code correct? What happens when a typeahead is hidden and users list is updated a couple of times? It seems like we probably need an additional flag instead of using frozenThreadMembers for two purposes: storing members and as a flag. |
| web/chat/typeahead-tooltip.react.js | ||
| 51–53 ↗ | (On Diff #20514) | We should avoid a magic value (empty array) that has a special meaning. Instead we should have a function that resets frozen members. |
| web/input/input-state.js | ||
|---|---|---|
| 39 ↗ | (On Diff #20514) | I feel like there's no better place for it. We can't keep it inside tooltip component as it's rendered based on SuggestedUsers (that are chosen from ThreadMembers - now frozen). ConnectedChatInputbar selects threadMembers so its state feels like natural place for keeping frozen copy. |
Comment Actions
Added flag keepUpdatingThreadMembers which is set false on mounting tooltip. It causes users to threadMemebrs to get frozen.
Comment Actions
We're really close!
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 591–602 ↗ | (On Diff #20627) | This is correct but can be made more efficient. Currently, we always have to rely on an effect to update the members, but the effect isn't necessary at all. To avoid the effect, we can do two things:
These two changes can be made in setTypeaheadState which will make it more performant and easier to use. |
| 592 ↗ | (On Diff #20627) | Maybe rename to areThreadMembersFrozen? |
Comment Actions
We've talked about that issue with Tomek. We've tried to find better solution, but it seems hard and we don't get huge benefits here.
Tomek has brought up issue, that calling useEffect that changes state will cause rerender of component. It's single rerender and every value above is memoised, so it shouldn't be huge overhead.
We've decided to go with current solution then.