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

feat(footer): connected indicator added under chatbar (AP-356) #1330

Merged
merged 6 commits into from
Feb 14, 2022
Merged

Conversation

jpanay
Copy link
Contributor

@jpanay jpanay commented Feb 4, 2022

What this PR does 📖
Shows if peer is connected or disconnected under chatbar
Which issue(s) this PR fixes 🔨
AP-356

Special notes for reviewers 🗒️

Additional comments 🎤

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 4, 2022
@phillsatellite
Copy link
Contributor

Should indicator appear if user has no friends added?
Screen Shot 2022-02-04 at 2 26 59 PM

@phillsatellite phillsatellite added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 4, 2022
@jpanay
Copy link
Contributor Author

jpanay commented Feb 4, 2022 via email

Copy link
Contributor

@WanderingHogan WanderingHogan left a comment

Choose a reason for hiding this comment

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

Can you:

  1. Split footer into a template, less, and vue file
  2. Create a is-connected component
  3. Add english to locales (in this repo, not the standalone repo)

@jpanay jpanay requested a review from WanderingHogan February 7, 2022 10:46
@stavares843 stavares843 added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Feb 7, 2022
@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Feb 8, 2022
@WanderingHogan
Copy link
Contributor

/rebase

@WanderingHogan WanderingHogan removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 8, 2022
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

Gravacao.do.ecra.2022-02-08.as.18.56.30.mov

says hogan is not connected on hogan's chat
change chat
still says hogan is not connected until page fully loads

@stavares843 stavares843 removed the QA tested One QA Person has tested - Needs QA Lead review still label Feb 8, 2022
@stavares843
Copy link
Member

stavares843 commented Feb 8, 2022

Gravacao.do.ecra.2022-02-08.as.19.03.52.mov

the connected/not connected updates fine

I closed the tab of user B and updated user A saying it was no longer connected

but then returned error on user A
Captura de ecrã 2022-02-08, às 19 05 40

and after reloading the page says user B is connected again

Captura de ecrã 2022-02-08, às 19 08 34

but the tab of user B was closed and not reopened

@stavares843 stavares843 added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 8, 2022
@netlify
Copy link

netlify bot commented Feb 10, 2022

✔️ Yeeeehaw, deploy preview is ready!

🔨 Explore the source changes: 6d0362d

🔍 Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/620821253aa16d0007edb28a

😎 Browse the preview: https://deploy-preview-1330--adoring-edison-dbcef8.netlify.app

@jpanay
Copy link
Contributor Author

jpanay commented Feb 10, 2022

Gravacao.do.ecra.2022-02-08.as.19.03.52.mov
the connected/not connected updates fine

I closed the tab of user B and updated user A saying it was no longer connected

but then returned error on user A Captura de ecrã 2022-02-08, às 19 05 40

and after reloading the page says user B is connected again

Captura de ecrã 2022-02-08, às 19 08 34

but the tab of user B was closed and not reopened

I'm trying to replicate this but I'm not getting that peer not connected error. I'm opening both tabs, waiting for connection, and closing one, but can't seem to get it to throw that error.

@jpanay
Copy link
Contributor Author

jpanay commented Feb 10, 2022

Gravacao.do.ecra.2022-02-08.as.19.03.52.mov
the connected/not connected updates fine
I closed the tab of user B and updated user A saying it was no longer connected
but then returned error on user A Captura de ecrã 2022-02-08, às 19 05 40
and after reloading the page says user B is connected again
Captura de ecrã 2022-02-08, às 19 08 34
but the tab of user B was closed and not reopened

I'm trying to replicate this but I'm not getting that peer not connected error. I'm opening both tabs, waiting for connection, and closing one, but can't seem to get it to throw that error.

Maybe I fixed it inadvertently with a few changes I made, can you test it again

@phillsatellite
Copy link
Contributor

I tested again, I got same error as @stavares843 after attempting to message user B after they disconnected, the only thing that was different on mine was when I reloaded page it still said User B was not connected
Screen Shot 2022-02-10 at 11 47 00 AM

@jpanay
Copy link
Contributor Author

jpanay commented Feb 10, 2022

I tested again, I got same error as @stavares843 after attempting to message user B after they disconnected, the only thing that was different on mine was when I reloaded page it still said User B was not connected Screen Shot 2022-02-10 at 11 47 00 AM

3 machines, 3 different results, ain't I lucky gal 😂

@jpanay
Copy link
Contributor Author

jpanay commented Feb 10, 2022

I tested again, I got same error as @stavares843 after attempting to message user B after they disconnected, the only thing that was different on mine was when I reloaded page it still said User B was not connected Screen Shot 2022-02-10 at 11 47 00 AM

Did you pull down the changes I made?

@phillsatellite
Copy link
Contributor

Member

I did 🔨

@WanderingHogan
Copy link
Contributor

@jpanay The error is here:
image

typingNotifyHandler in the chatbar.vue file.

const activePeer = this.$WebRTC.getPeer(activeFriend.address) can come back as undefined

In lieue of digging into getPeer in the webrtc library, for now, please:

In the chatbar.vue file wrap the getPeer call in a try catch,
Screen Shot 2022-02-10 at 8 01 12 PM

@WanderingHogan
Copy link
Contributor

@jpanay I can reproduce the error on my end

@jpanay
Copy link
Contributor Author

jpanay commented Feb 11, 2022

@jpanay I can reproduce the error on my end

Yea I really don't know why I wasn't able to if all three of you can

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Feb 11, 2022
@WanderingHogan
Copy link
Contributor

looks good

@stavares843
Copy link
Member

stavares843 commented Feb 12, 2022

Captura de ecrã 2022-02-12, às 02 28 41

i have 2 users, they were both connected, I deactivated internet, and even now both have internet, still says one of them is not connected

full flow was

  • user A added user B
  • user B sent message to user A
  • user A sent message to user B
  • go to console network settings on user A and select offline

Captura de ecrã 2022-02-12, às 02 30 12

  • user B sent message to user A - it sends
  • user A sent message to user B - doesn't send, there's no internet on that user
  • remove offline setting
  • user A sent message to user B - sends successfully
  • still shows user B not connected on user A side

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Feb 12, 2022
@stavares843
Copy link
Member

/rebase

@jpanay
Copy link
Contributor Author

jpanay commented Feb 12, 2022

Captura de ecrã 2022-02-12, às 02 28 41

i have 2 users, they were both connected, I deactivated internet, and even now both have internet, still says one of them is not connected

full flow was

  • user A added user B
  • user B sent message to user A
  • user A sent message to user B
  • go to console network settings on user A and select offline
Captura de ecrã 2022-02-12, às 02 30 12
  • user B sent message to user A - it sends
  • user A sent message to user B - doesn't send, there's no internet on that user
  • remove offline setting
  • user A sent message to user B - sends successfully
  • still shows user B not connected on user A side

Could you make a separate ticket for this, it's a webrtc issue beyond the scope of this PR

@stavares843
Copy link
Member

the ticket which this tickets is expected to implement says the following:

''Show an indication under the chat input if the peer is connected/disconnected, might require checking/watching for events that don't exist?''

so, one of those events, would be a disconnected event as well, such as without internet

is not outside the scope, because the PR should include that doesn't matter if is or not a webrtc issue

but let me know cc @WanderingHogan

@WanderingHogan
Copy link
Contributor

I think that this is a problem with devtools in chrome - i dont think all connection types are getting closed when you disable the connection.

This acts differently when you are on two different devices. Eventually the webrtc connection times out and shows both users are offline. I think that there are problems managing the webrtc connection that should be included in future tickets but we need to debug some things first.

If this ticket works otherwise and doesn't cause any crashes i think it can go in.

@stavares843 stavares843 merged commit 6372b2e into dev Feb 14, 2022
@stavares843 stavares843 deleted the AP-356 branch February 14, 2022 20:54
@github-actions github-actions bot removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 14, 2022
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.

5 participants