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

Chatrix peeking w/ guest account #193

Merged
merged 9 commits into from
Mar 16, 2023

Conversation

ashfame
Copy link
Member

@ashfame ashfame commented Feb 28, 2023

This draft PR (not meant for merge) highlights the differences needed in Chatrix to make guest account peeking work.

This would require more changes than current's peeking PR to Hydrogen (Automattic/hydrogen-web#7) So this branch in Chatrix uses the linked branch in Hydrogen as its dependency for Hydrogen.

To run: Pull the branch and don't forget to yarn install and yarn build

chatrix_guest_session_peeking

@ashfame ashfame self-assigned this Feb 28, 2023
@ashfame ashfame changed the base branch from main to hydrogen_peeking_dependency February 28, 2023 16:19
@akirk
Copy link
Member

akirk commented Mar 1, 2023

I edited your description so that it contains Automattic/hydrogen-web#7 in verbatim, thus Github can create a link into that PR.

@ashfame ashfame force-pushed the hydrogen_peeking_dependency_guest_login branch from 1f54741 to 476875f Compare March 1, 2023 16:38
@ashfame ashfame force-pushed the hydrogen_peeking_dependency branch from 01065ad to e3ad0a3 Compare March 1, 2023 19:04
@ashfame ashfame force-pushed the hydrogen_peeking_dependency_guest_login branch from 476875f to a1e8b6c Compare March 1, 2023 20:33
@ashfame
Copy link
Member Author

ashfame commented Mar 1, 2023

The branch is now functional except the error that you see on the top ruining it asthetically. Guest user session isn't allowed to create a filter, hence this error.

This brings us to the question of how does Hydrogen want to treat guest user session different than a regular one, and how is that best done, which is what I am currently exploring. I would prefer to do it within Chatrix for now, if possible, before we get to the topic of how does "guest user session" look like in Hydrogen.

@ashfame
Copy link
Member Author

ashfame commented Mar 3, 2023

With more changes done in Automattic/hydrogen-web#7 this is now functional without the error in the screenshot above. Sync runs for guest user session and should, for cases when guest user session is used to join a room.

@ashfame ashfame marked this pull request as ready for review March 3, 2023 15:56
@ashfame ashfame requested a review from psrpinto March 3, 2023 15:56
@ashfame ashfame mentioned this pull request Mar 6, 2023
4 tasks
@ashfame ashfame force-pushed the hydrogen_peeking_dependency_guest_login branch from f7fa2f0 to dc1362b Compare March 7, 2023 09:38
@psrpinto
Copy link
Member

psrpinto commented Mar 9, 2023

Here are some issues I found while trying this:

  1. Sometimes chatrix doesn't load and the block will say Something went wrong, this.session.isGuest is not a function

  2. Sometimes some assets fail to load, CSS or fonts, though this could be unrelated to this PR (though I don't think I had experienced it before)

  3. Chatrix doesn't seem to remember anymore which session is open. After logging in as guest, then logging with my regular account, I will have two sessions in the session picker. If I open one session, then refresh the page, I will go back to the session picker.

@ashfame
Copy link
Member Author

ashfame commented Mar 9, 2023

@psrpinto

  1. Sometimes chatrix doesn't load and the block will say Something went wrong, this.session.isGuest is not a function

This issue has been fixed!

  1. Sometimes some assets fail to load, CSS or fonts, though this could be unrelated to this PR (though I don't think I had experienced it before)

I haven't encountered this issue even once so need to reproduce it before I can debug it.

  1. Chatrix doesn't seem to remember anymore which session is open.

Remembering last session is part of the "last url restoring" functionality which is now disabled for single room mode. We also talked about how its safe to assume for Chatrix context to not have multiple sessions. Have you identified a use case which justifies support for multiple sessions in a single Chatrix instance? If so, can you help me with the user story for it?

@ashfame
Copy link
Member Author

ashfame commented Mar 16, 2023

@psrpinto For point 3 above, if you have indeed identified the need for multiple sessions in a single Chatrix instance opposed to our assumption, can you please file an issue with the user story? We can handle that in a follow up PR. For now, I am taking this PR forward ✌️

@psrpinto
Copy link
Member

psrpinto commented Mar 16, 2023

  1. Sometimes some assets fail to load
    I haven't encountered this issue

Maybe this was related to the this.session.isGuest is not a function error. Please ignore.

  1. Chatrix doesn't seem to remember anymore which session is open.

I opened #200 to track this.

Base automatically changed from hydrogen_peeking_dependency to main March 16, 2023 12:35
@ashfame ashfame merged commit 51e9703 into main Mar 16, 2023
@ashfame ashfame deleted the hydrogen_peeking_dependency_guest_login branch March 16, 2023 12:35
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.

3 participants