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.

Sign up for GitHub

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

Jump to bottom

Failed to load timeline when viewing a room scrolled back in time and then swapping to another room #4224

Closed
lukebarnard1 opened this issue Jun 7, 2017 · 3 comments
Closed

Failed to load timeline when viewing a room scrolled back in time and then swapping to another room #4224

lukebarnard1 opened this issue Jun 7, 2017 · 3 comments

Comments

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Jun 7, 2017 •
edited
Loading

  • Go to a room and jump to a message in the past
  • Swap to any other room
  • Swap back to the original room
  • Swap to any other room
  • See Failed to load timeline position
@uhoreg uhoreg mentioned this issue Jun 7, 2017
Closed
@richvdh
Copy link
Member

richvdh commented Jun 7, 2017

Another repro case:

  • Do a search,
  • follow a result,
  • switch to another room,
  • observe error

@richvdh
Copy link
Member

richvdh commented Jun 7, 2017

From #riot-dev:

this is happening because MatrixChat's _onRoomViewStoreUpdated is firing before onAction, which means it is trying to mount the new RoomView while state.initialEventId still points to an event in the old room

I guess the problem here is that initialEventId is in the state, so gets stuck, but I don't know how else you are meant to pass one-off parameters into subcomponents

in general, setting state.currentRoomId without setting all the other things that get set in _viewRoom feels like it's going to go wrong

lukebarnard1 pushed a commit to matrix-org/matrix-react-sdk that referenced this issue Jun 8, 2017
Control currently viewied event via RoomViewStore
d3cf78f 
Fix for element-hq/element-web#4224

Due to the way `MatrixChat` does a state update when the `view_room` dispatch fires and a second update when `RoomViewStore` sends an update, the current event ID and room ID were becoming out of sync. The solution devised was to have the event ID managed by the `RoomViewStore` itself and do any defaulting there (for when we revisit a room that we saved scroll state for previously).

This required a few changes:
 - The addition of `update_scroll_state` in `RoomViewStore` allows the `RoomView` to save scroll state for a room before swapping to another one. Previously the caching of scroll state was done in `RoomView`.
 - The `view_room` dispatch now accepts an `event_id`, which dictates which event is supposed to be scrolled to in the `MessagePanel` when a new room is viewed. It also accepts `event_offset`, but currently, this isn't passed in by a dispatch in the app, but it is clobbered when loading the default position when an `event_id` isn't specified. Finally, `highlighted` was added to distinguish whether the initial event being scrolled to is also highlighted. This flag is also used by `viewRoom` in `MatrixChat` in order to decide whether to `notifyNewScreen` with the specified `event_id`.
@lukebarnard1 lukebarnard1 mentioned this issue Jun 8, 2017
Merged
@ara4n
Copy link
Member

ara4n commented Jun 8, 2017

tested the repro case above and it seems to be working now - yay! but the performance is at least twice as worse for rooms which are scrolled up; boo :(( have filed #4255 for this one though.

@ara4n ara4n closed this as completed Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ara4n @richvdh @lukebarnard1