Skip to content

Fix race condition of "not in chat"#1652

Merged
PorterNan merged 24 commits intomainfrom
jinan/fix-not-in-chat
Mar 15, 2022
Merged

Fix race condition of "not in chat"#1652
PorterNan merged 24 commits intomainfrom
jinan/fix-not-in-chat

Conversation

@PorterNan
Copy link
Copy Markdown
Contributor

@PorterNan PorterNan commented Mar 15, 2022

What

https://skype.visualstudio.com/SPOOL/_queries/query/8f724fc4-2b1b-4030-9471-9a64e1c8a7d0/

We are pulling the initial data when we are creating the chat adapter, which should not be part of adapter(adapter itself doesn't control when to call functions, someone external should do it)

remove the fetchInitialData() function call in creation of chat adapter, and we will highly depend on ChatScreen or external caller to do this (what we are currently doing already)

The above error is happening because:

  1. we create callAdapter and chatAdapter when we are not part of the thread (the user hasn't been admitted yet)
  2. createChatAdapter calls fetchInitialData, which throws "not in chat" error
  3. the user is admitted to chat later when someone in teams click the button

This scenario means we should not make any request when we are creating adapter(we should not assume we have all the permission), we should depend on external caller to make the final request

How Tested

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

@github-actions
Copy link
Copy Markdown
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

2 similar comments
@github-actions
Copy link
Copy Markdown
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

@github-actions
Copy link
Copy Markdown
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

@PorterNan
Copy link
Copy Markdown
Contributor Author

One more interesting topic to be brought up, looks like we are adding this to avoid flaky tests(which is working pretty well), and now we might need some other way to work this out
#1168

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@PorterNan
Copy link
Copy Markdown
Contributor Author

I don't understand why this PR fixes the issue -- why are we sure that ChatScreen will only render after the user is admitted to the chat thread?

We can't, but at least it leaves you the flexibility to do this, and our meeting composite needs this gap not to fetch the data when chat adapter is created

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

Comment thread packages/react-composites/src/composites/ChatComposite/ChatScreen.tsx Outdated
@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@PorterNan PorterNan merged commit 6ca4a19 into main Mar 15, 2022
@PorterNan PorterNan deleted the jinan/fix-not-in-chat branch March 15, 2022 23:15
PorterNan added a commit that referenced this pull request Mar 15, 2022
* Fix race condition of "not in chat"

* Add comments about fetchInitialData
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