Peeking unknown rooms#1037
Open
ashfame wants to merge 10 commits into
Open
Conversation
…e by fetching messages and storing in the indexed db tables
…ligned with Matrix spec
Co-authored-by: Paulo Pinto <paulo.pinto@automattic.com>
…nd loading things async and updating view as we do This shows a spinner on UnknownRoomView stating "checking preview capability" and goes away when done and if room is world_readable, then renders the timeline with the last 100 messages
Contributor
|
Related: #719 |
- UnknownRoomViewModel now receives a promise using which it can display a spinner to the user indicating we are checking whether the preview is possible for this room and updates the model to WorldReadableRoomViewModel - kind is now changed to "preview" for WorldReadableRooms - data stored is now deleted when navigating away to another room by clicking on it
This was referenced Feb 25, 2023
Author
|
@MidhunSureshR This is now finished and ready for your review :) |
Member
MidhunSureshR
left a comment
MidhunSureshR
left a comment
There was a problem hiding this comment.
Doing an initial review; mostly looks good 👍
| if (room.isArchived) { | ||
| this._composerVM = this.track(new ArchivedViewModel(this.childOptions({archivedRoom: room}))); | ||
| } else { | ||
| } else if (!room.isWorldReadable) { |
Member
There was a problem hiding this comment.
I don't see this property (isWorldReadable) added to Room anywhere?
| onClick: () => vm.join(), | ||
| disabled: vm => vm.busy, | ||
| }, vm.i18n`Join room`), | ||
| t.br(), |
Member
There was a problem hiding this comment.
Can we use CSS to add some margin/padding instead of the br element?
| disabled: vm => vm.busy, | ||
| }, vm.i18n`Join room`), | ||
| t.br(), | ||
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "checkingPreviewCapability"}, [ |
Member
There was a problem hiding this comment.
Suggested change
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "checkingPreviewCapability"}, [ | |
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "UnknownRoomVIew__PreviewIndicator"}, [ |
Following BEM
| disabled: vm => vm.busy, | ||
| }, vm.i18n`Join room`), | ||
| t.br(), | ||
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "checkingPreviewCapability"}, [ |
Member
There was a problem hiding this comment.
Suggested change
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "checkingPreviewCapability"}, [ | |
| t.if(vm => vm.checkingPreviewCapability, t => t.div({className: "UnknownRoomVIew__PreviewIndicator"}, [ |
Following BEM
| }); | ||
| } | ||
|
|
||
| async _fetchWorldReadableRoomEvents(roomId, limit = 30, dir = 'b', end = null, log = null) { |
Member
There was a problem hiding this comment.
Can you add a typescript const enum for dir? You can add it in HomeSeverApi.
| log.set("id", roomId); | ||
| let options = { | ||
| limit: limit, | ||
| dir: 'b', |
Member
There was a problem hiding this comment.
Suggested change
| dir: 'b', | |
| dir |
Comment on lines
+1077
to
+1086
| const resp = await this._hsApi.currentState(roomId).response(); | ||
| for ( let i=0; i<resp.length; i++ ) { | ||
| if ( resp[i].type === 'm.room.name') { | ||
| summary["name"] = resp[i].content.name; | ||
| } else if ( resp[i].type === 'm.room.canonical_alias' ) { | ||
| summary["canonicalAlias"] = resp[i].content.alias; | ||
| } else if ( resp[i].type === 'm.room.avatar' ) { | ||
| summary["avatarUrl"] = resp[i].content.url; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
We usually use for-of loops whenever possible. Also can you refactor the if-else ladder here into a switch-case?
| return new UnknownRoomViewModel(this.childOptions({ | ||
| roomIdOrAlias, | ||
| session: this._client.session, | ||
| isWorldReadablePromise: isWorldReadablePromise |
Member
There was a problem hiding this comment.
Suggested change
| isWorldReadablePromise: isWorldReadablePromise | |
| isWorldReadablePromise |
| })); | ||
| } | ||
|
|
||
| async _createWorldReadableRoomViewModel(roomIdOrAlias) { |
Member
There was a problem hiding this comment.
Is this method used anywhere?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
👋 This PR introduces the preview/peeking functionality when you load an unknown room by directly going to the URL
http://localhost:3000/#/session/3442497412902854/room/!wOlkWNmgkAZFxbTaqj%3Amatrix.orgWhen you land on this page, we check whether the room supports preview/peeking i.e.
world_readablehistory visibility by showing a spinner in the UI:Then if the room isn't preview-able, the spinner goes away and we are left with Unknown Room View exactly how it is prior to this PR. But in case, if its possible to preview the room messages, we fetch the 100 most recent messages from the homeserver and store them in indexedDB tables (
timelineEventsandtimelineFragments) as they would have been stored via sync response for a regular room. On every page load, the old messages stored in both tables are deleted and freshly fetched 100 messages are shown. Also when you are navigating away from a room, by clicking on another room from left sidebar, we delete the data from both tables that were fetched in context of peeking.Currently back-scrolling works but it doesn't sync new messages i.e. doesn't show more messages as they come in, in the room. Also, the users are currently displayed as their matrix identity.
Looking forward to your review and happy to make any changes as needed to get this merged :)