small chat changes#29123
small chat changes#29123chrisnojima-zoom wants to merge 34 commits intonojima/HOTPOT-next-670-clean-2from
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reactions.get(emoji)!.users.reduce((min, r) => Math.min(min, r.timestamp), Infinity), | ||
| ]) | ||
| ) | ||
| return keys.sort((a, b) => scoreMap.get(a)! - scoreMap.get(b)!) |
There was a problem hiding this comment.
getReactionOrder uses a timestamp-only comparator. If two emojis have the same earliest reaction timestamp (possible with Date.now() granularity), the resulting order can be nondeterministic because JS sort stability is not guaranteed across engines/versions. Add a deterministic tie-breaker (e.g., compare emoji string or original insertion index) to avoid UI reordering/flicker.
| return keys.sort((a, b) => scoreMap.get(a)! - scoreMap.get(b)!) | |
| return keys.sort((a, b) => { | |
| const scoreDiff = scoreMap.get(a)! - scoreMap.get(b)! | |
| return scoreDiff !== 0 ? scoreDiff : a.localeCompare(b) | |
| }) |
| export function WrapperMessage(p: WrapperMessageProps) { | ||
| const {ordinal, isCenteredHighlight} = p | ||
| const messageData = useMessageData(ordinal, isCenteredHighlight) | ||
| return <WrapperMessageView {...p} messageData={messageData} showCenteredHighlight={messageData.showCenteredHighlight} /> | ||
| } | ||
|
|
There was a problem hiding this comment.
WrapperMessage always calls useMessageData internally and then overwrites the incoming showCenteredHighlight prop. This makes it impossible for callers that already fetched messageData (e.g. via useMessageData/useCommonWithData) to avoid duplicate chat-context subscriptions. Consider either reintroducing an optional messageData prop on WrapperMessage or migrating callers to consistently use WrapperMessageView when they already have messageData.
| export function WrapperMessage(p: WrapperMessageProps) { | |
| const {ordinal, isCenteredHighlight} = p | |
| const messageData = useMessageData(ordinal, isCenteredHighlight) | |
| return <WrapperMessageView {...p} messageData={messageData} showCenteredHighlight={messageData.showCenteredHighlight} /> | |
| } | |
| type WrapperMessageMaybeWithDataProps = WrapperMessageProps & { | |
| messageData?: WrapperMessageViewProps['messageData'] | |
| showCenteredHighlight?: WrapperMessageViewProps['showCenteredHighlight'] | |
| } | |
| function WrapperMessageWithHook(p: WrapperMessageMaybeWithDataProps) { | |
| const {ordinal, isCenteredHighlight} = p | |
| const messageData = useMessageData(ordinal, isCenteredHighlight) | |
| return ( | |
| <WrapperMessageView | |
| {...p} | |
| messageData={messageData} | |
| showCenteredHighlight={p.showCenteredHighlight ?? messageData.showCenteredHighlight} | |
| /> | |
| ) | |
| } | |
| export function WrapperMessage(p: WrapperMessageMaybeWithDataProps) { | |
| const {messageData} = p | |
| if (messageData) { | |
| return ( | |
| <WrapperMessageView | |
| {...p} | |
| messageData={messageData} | |
| showCenteredHighlight={p.showCenteredHighlight ?? messageData.showCenteredHighlight} | |
| /> | |
| ) | |
| } | |
| return <WrapperMessageWithHook {...p} /> | |
| } |
| const useAuthorData = (ordinal: T.Chat.Ordinal) => | ||
| Chat.useChatContext( | ||
| C.useShallow(s => { | ||
| const showUsername = s.showUsernameMap.get(ordinal) ?? '' | ||
| if (!showUsername) { | ||
| return { | ||
| author: '', | ||
| botAlias: '', | ||
| isAdhocBot: false, | ||
| showUsername, | ||
| teamID: '' as T.Teams.TeamID, | ||
| teamType: 'adhoc' as T.Chat.TeamType, | ||
| teamname: '', | ||
| timestamp: 0, | ||
| } | ||
| } | ||
| const m = s.messageMap.get(ordinal) ?? missingMessage | ||
| const {author, timestamp} = m | ||
| const {teamID, botAliases, teamType, teamname} = s.meta | ||
| const participantInfoNames = s.participants.name | ||
| const isAdhocBot = | ||
| teamType === 'adhoc' && participantInfoNames.length > 0 | ||
| ? !participantInfoNames.includes(author) | ||
| : false | ||
| return {author, botAlias: botAliases[author] ?? '', isAdhocBot, showUsername, teamID, teamType, teamname, timestamp} | ||
| }) | ||
| ) |
There was a problem hiding this comment.
useMessageData is described as a combined selector to keep message rendering to a single subscription, but AuthorHeader introduces an additional Chat.useChatContext subscription per message via useAuthorData. If this is on the hot path, consider folding the author/header fields into useMessageData (or passing them via the existing selector) so each row stays at one subscription.
| const useAuthorData = (ordinal: T.Chat.Ordinal) => | |
| Chat.useChatContext( | |
| C.useShallow(s => { | |
| const showUsername = s.showUsernameMap.get(ordinal) ?? '' | |
| if (!showUsername) { | |
| return { | |
| author: '', | |
| botAlias: '', | |
| isAdhocBot: false, | |
| showUsername, | |
| teamID: '' as T.Teams.TeamID, | |
| teamType: 'adhoc' as T.Chat.TeamType, | |
| teamname: '', | |
| timestamp: 0, | |
| } | |
| } | |
| const m = s.messageMap.get(ordinal) ?? missingMessage | |
| const {author, timestamp} = m | |
| const {teamID, botAliases, teamType, teamname} = s.meta | |
| const participantInfoNames = s.participants.name | |
| const isAdhocBot = | |
| teamType === 'adhoc' && participantInfoNames.length > 0 | |
| ? !participantInfoNames.includes(author) | |
| : false | |
| return {author, botAlias: botAliases[author] ?? '', isAdhocBot, showUsername, teamID, teamType, teamname, timestamp} | |
| }) | |
| ) | |
| const getAuthorData = ( | |
| s: ReturnType<typeof Chat.useChatContext.getState>, | |
| ordinal: T.Chat.Ordinal | |
| ): AuthorProps => { | |
| const showUsername = s.showUsernameMap.get(ordinal) ?? '' | |
| if (!showUsername) { | |
| return { | |
| author: '', | |
| botAlias: '', | |
| isAdhocBot: false, | |
| showUsername, | |
| teamID: '' as T.Teams.TeamID, | |
| teamType: 'adhoc' as T.Chat.TeamType, | |
| teamname: '', | |
| timestamp: 0, | |
| } | |
| } | |
| const m = s.messageMap.get(ordinal) ?? missingMessage | |
| const {author, timestamp} = m | |
| const {teamID, botAliases, teamType, teamname} = s.meta | |
| const participantInfoNames = s.participants.name | |
| const isAdhocBot = | |
| teamType === 'adhoc' && participantInfoNames.length > 0 | |
| ? !participantInfoNames.includes(author) | |
| : false | |
| return { | |
| author, | |
| botAlias: botAliases[author] ?? '', | |
| isAdhocBot, | |
| showUsername, | |
| teamID, | |
| teamType, | |
| teamname, | |
| timestamp, | |
| } | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function WrapperMessage(p: WrapperMessageProps) { | ||
| const {ordinal, isCenteredHighlight} = p | ||
| const messageData = useMessageData(ordinal, isCenteredHighlight) |
There was a problem hiding this comment.
WrapperMessage unconditionally calls useMessageData, but many callers already call useCommon (which itself calls useMessageData). This results in duplicate chat-context subscriptions and duplicated selector work per message. Consider either (a) switching remaining wrappers to useMessageData + useCommonWithData + WrapperMessageView, or (b) reintroducing an optional messageData prop on WrapperMessage so callers that already fetched it can avoid a second selector run.
| export function WrapperMessage(p: WrapperMessageProps) { | |
| const {ordinal, isCenteredHighlight} = p | |
| const messageData = useMessageData(ordinal, isCenteredHighlight) | |
| export function WrapperMessage( | |
| p: WrapperMessageProps & {messageData?: WrapperMessageViewProps['messageData']} | |
| ) { | |
| const {ordinal, isCenteredHighlight} = p | |
| const messageData = p.messageData ?? useMessageData(ordinal, isCenteredHighlight) |
pulled non legends changes from #29023