Feedback for ChatList #79

TechnicallyWilliams · 2024-02-27T15:52:17Z

This PR addresses some comments from Gavin on ChatList:

Completed:

todo: ChatList no longer references sign-in state to execute any logic.
todo: ChatListState state var is now only handler subscribed to statefulgraphchatlistclient state changes. UseEffect now relies on updates to ChatListState.
todo: implement offStateChange
todo: add doc to indicate why we are not adding deps
"todo": remove sign-in state check from chatlistItem

…with callback

…initialization

packages/mgt-chat/src/components/ChatList/ChatList.tsx

plasne · 2024-02-27T22:19:45Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

seekdavidlee · 2024-02-27T22:26:53Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

what about when user is switched?

plasne · 2024-02-27T22:37:04Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

what about when user is switched?

That's a good question, but when that happens is there not a "sign-out" and "sign-in" event? And is the ChatList component recreated when that happens?

seekdavidlee · 2024-02-27T22:49:01Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

what about when user is switched?

That's a good question, but when that happens is there not a "sign-out" and "sign-in" event? And is the ChatList component recreated when that happens?

we are still hooked into AccountChange event, so it is possible ChatList is still active I suppose, under StatefulGraphChatListClient.

Providers.globalProvider.onActiveAccountChanged(this.onActiveAccountChanged);

plasne · 2024-02-27T23:23:09Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

what about when user is switched?

That's a good question, but when that happens is there not a "sign-out" and "sign-in" event? And is the ChatList component recreated when that happens?

we are still hooked into AccountChange event, so it is possible ChatList is still active I suppose, under StatefulGraphChatListClient.

Providers.globalProvider.onActiveAccountChanged(this.onActiveAccountChanged);

I think we should test it. It would certainly be cleaner if it did do a sign-out and then back in.

samples/react-chat/src/App.tsx

seekdavidlee · 2024-02-28T13:51:58Z

We probably don't need the renewal locking currentUserId anymore since it cannot change now.

what about when user is switched?

That's a good question, but when that happens is there not a "sign-out" and "sign-in" event? And is the ChatList component recreated when that happens?

we are still hooked into AccountChange event, so it is possible ChatList is still active I suppose, under StatefulGraphChatListClient.
Providers.globalProvider.onActiveAccountChanged(this.onActiveAccountChanged);

I think we should test it. It would certainly be cleaner if it did do a sign-out and then back in.

I tested. Here, I switch between the 2 signed in users but we stay on the same page.

Co-authored-by: Dexter Williams <dexterwilliams04@gmail.com>

github-actions · 2024-02-28T14:41:54Z

Package	Line Rate	Branch Rate	Health
mgt-chat.src.statefulClient	100%	100%	✔
mgt-chat.src.utils	100%	89%	✔
mgt-components.dist.es6.components.mgt-agenda.src.components.mgt-agenda	14%	100%	❌
mgt-components.dist.es6.components.mgt-contact.src.components.mgt-contact	62%	100%	➖
mgt-components.dist.es6.components.mgt-file-list.mgt-file-upload.src.components.mgt-file-list.mgt-file-upload	43%	100%	❌
mgt-components.dist.es6.components.mgt-file-list.src.components.mgt-file-list	62%	100%	➖
mgt-components.dist.es6.components.mgt-file.src.components.mgt-file	61%	100%	➖
mgt-components.dist.es6.components.mgt-get.src.components.mgt-get	19%	100%	❌
mgt-components.dist.es6.components.mgt-login.src.components.mgt-login	64%	100%	➖
mgt-components.dist.es6.components.mgt-messages.src.components.mgt-messages	66%	100%	➖
mgt-components.dist.es6.components.mgt-organization.src.components.mgt-organization	46%	100%	❌
mgt-components.dist.es6.components.mgt-people-picker.src.components.mgt-people-picker	56%	100%	➖
mgt-components.dist.es6.components.mgt-people.src.components.mgt-people	72%	100%	➖
mgt-components.dist.es6.components.mgt-person-card.src.components.mgt-person-card	59%	33%	➖
mgt-components.dist.es6.components.mgt-person.src.components.mgt-person	53%	100%	➖
mgt-components.dist.es6.components.mgt-picker.src.components.mgt-picker	78%	100%	✔
mgt-components.dist.es6.components.mgt-planner.src.components.mgt-planner	55%	100%	➖
mgt-components.dist.es6.components.mgt-profile.src.components.mgt-profile	39%	100%	❌
mgt-components.dist.es6.components.mgt-search-box.src.components.mgt-search-box	83%	100%	✔
mgt-components.dist.es6.components.mgt-search-results.src.components.mgt-search-results	56%	100%	➖
mgt-components.dist.es6.components.mgt-tasks-base.src.components.mgt-tasks-base	86%	100%	✔
mgt-components.dist.es6.components.mgt-taxonomy-picker.src.components.mgt-taxonomy-picker	74%	100%	➖
mgt-components.dist.es6.components.mgt-teams-channel-picker.src.components.mgt-teams-channel-picker	62%	100%	➖
mgt-components.dist.es6.components.mgt-theme-toggle.src.components.mgt-theme-toggle	74%	100%	➖
mgt-components.dist.es6.components.mgt-todo.src.components.mgt-todo	78%	100%	✔
mgt-components.dist.es6.components.src.components	86%	100%	✔
mgt-components.dist.es6.components.sub-components.mgt-arrow-options.src.components.sub-components.mgt-arrow-options	76%	100%	✔
mgt-components.dist.es6.components.sub-components.mgt-dot-options.src.components.sub-components.mgt-dot-options	29%	100%	❌
mgt-components.dist.es6.components.sub-components.mgt-flyout.src.components.sub-components.mgt-flyout	40%	100%	❌
mgt-components.dist.es6.components.sub-components.mgt-spinner.src.components.sub-components.mgt-spinner	92%	100%	✔
mgt-components.dist.es6.graph.src.graph	36%	100%	❌
mgt-components.dist.es6.src	100%	100%	✔
mgt-components.dist.es6.styles.src.styles	73%	100%	➖
mgt-components.dist.es6.utils.src.utils	46%	100%	❌
mgt-components.src.components	84%	75%	✔
mgt-components.src.components.mgt-contact	68%	83%	➖
mgt-components.src.components.mgt-file	62%	100%	➖
mgt-components.src.components.mgt-file-list	46%	100%	❌
mgt-components.src.components.mgt-file-list.mgt-file-upload	49%	86%	❌
mgt-components.src.components.mgt-get	22%	100%	❌
mgt-components.src.components.mgt-messages	68%	100%	➖
mgt-components.src.components.mgt-organization	47%	100%	❌
mgt-components.src.components.mgt-person	84%	76%	✔
mgt-components.src.components.mgt-person-card	67%	48%	➖
mgt-components.src.components.mgt-picker	80%	100%	✔
mgt-components.src.components.mgt-profile	40%	100%	❌
mgt-components.src.components.mgt-tasks-base	87%	100%	✔
mgt-components.src.components.mgt-theme-toggle	100%	100%	✔
mgt-components.src.components.mgt-todo	79%	100%	✔
mgt-components.src.components.sub-components.mgt-flyout	72%	53%	➖
mgt-components.src.components.sub-components.mgt-spinner	100%	100%	✔
mgt-components.src.graph	40%	73%	❌
mgt-components.src.styles	92%	80%	✔
mgt-components.src.utils	82%	46%	✔
mgt-element.dist.es6.components.src.components	72%	74%	➖
mgt-element.dist.es6.mock.src.mock	91%	77%	✔
mgt-element.dist.es6.providers.src.providers	87%	83%	✔
mgt-element.dist.es6.src	91%	80%	✔
mgt-element.dist.es6.utils.src.utils	67%	73%	➖
mgt-element.src	93%	40%	✔
mgt-element.src.components	78%	100%	✔
mgt-element.src.mock	81%	56%	✔
mgt-element.src.providers	83%	91%	✔
mgt-element.src.utils	71%	90%	➖
Summary	60% (28031 / 46554)	71% (562 / 795)	➖

andrewDoing · 2024-02-28T16:01:16Z

samples/react-contoso/src/pages/ChatPage.tsx

@@ -143,7 +144,7 @@ const ChatPage: React.FunctionComponent = () => {
          </Dialog>
        </div>
        <div className={styles.side}>
-          <ChatListWrapper selectedChatId={chatId} onSelected={onChatSelected} onNewChat={onNewChat} />
+          {isSignedIn && <ChatListWrapper selectedChatId={chatId} onSelected={onChatSelected} onNewChat={onNewChat} />}


was this the intended implementation? The user would somehow need to know to conditionally render this based on isSignedIn. If it was a prop, we can do this conditional render or just short circuit within the component. Plus the user knows they must provide it

This is a common approach, LGTM

TechnicallyWilliams added 2 commits February 27, 2024 09:19

assume signed in; replace onstatechange with useEffect; wrap onclick …

4b10552

…with callback

remove references to provider for sign-in

f95c6f3

TechnicallyWilliams assigned seekdavidlee Feb 27, 2024

seekdavidlee added 6 commits February 27, 2024 10:05

Merge branch 'next/mgt-chat' into dexterw/chatlist-rework-state-strategy

96cb1f3

rework samples to check for logged in status, cleanup chatlistclient …

9bcf59c

…initialization

reset app id

a31c728

Merge branch 'next/mgt-chat' into dexterw/chatlist-rework-state-strategy

efcae58

remove just the chatId from session, not everything

813126c

remove signed in assertion in chatlistitem

620b621

seekdavidlee marked this pull request as ready for review February 27, 2024 20:45

seekdavidlee requested review from MattBillFred, plasne, kavithasreenath, andrewDoing and salujamanish February 27, 2024 20:46

rework chatListActions declaration

e4fdd74

plasne reviewed Feb 27, 2024

View reviewed changes

packages/mgt-chat/src/components/ChatList/ChatList.tsx Outdated Show resolved Hide resolved

plasne reviewed Feb 27, 2024

View reviewed changes

packages/mgt-chat/src/components/ChatList/ChatList.tsx Outdated Show resolved Hide resolved

fix dup subscription events from signalr

3da946a

remove duplicative

7f0ce80

TechnicallyWilliams commented Feb 28, 2024

View reviewed changes

samples/react-chat/src/App.tsx Outdated Show resolved Hide resolved

commit suggestion

c3fcccf

Co-authored-by: Dexter Williams <dexterwilliams04@gmail.com>

plasne approved these changes Feb 28, 2024

View reviewed changes

fix spelling

c77484e

andrewDoing reviewed Feb 28, 2024

View reviewed changes

andrewDoing approved these changes Feb 28, 2024

View reviewed changes

seekdavidlee merged commit ec95d3c into next/mgt-chat Feb 28, 2024
8 checks passed

seekdavidlee deleted the dexterw/chatlist-rework-state-strategy branch February 28, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feedback for ChatList #79

Feedback for ChatList #79

TechnicallyWilliams commented Feb 27, 2024 •

edited by seekdavidlee

Loading

plasne commented Feb 27, 2024

seekdavidlee commented Feb 27, 2024

plasne commented Feb 27, 2024

seekdavidlee commented Feb 27, 2024

plasne commented Feb 27, 2024

seekdavidlee commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

andrewDoing Feb 28, 2024

andrewDoing Feb 28, 2024

Feedback for ChatList #79

Feedback for ChatList #79

Conversation

TechnicallyWilliams commented Feb 27, 2024 • edited by seekdavidlee Loading

plasne commented Feb 27, 2024

seekdavidlee commented Feb 27, 2024

plasne commented Feb 27, 2024

seekdavidlee commented Feb 27, 2024

plasne commented Feb 27, 2024

seekdavidlee commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

andrewDoing Feb 28, 2024

Choose a reason for hiding this comment

andrewDoing Feb 28, 2024

Choose a reason for hiding this comment

TechnicallyWilliams commented Feb 27, 2024 •

edited by seekdavidlee

Loading