Conversation
mbeernut
commented
Mar 18, 2026
mbeernut
commented
- connect ti signalR
- auto update idle dialogue window
- autoscroll bottom on update
- fix chat list last message update
mbeernut
requested review from
DrJakiellSWO,
GitchalWoo,
HalilSWO,
MagdalenaKomudaSWO,
Tania-Roche-SWO,
dk1990,
jml-pl and
magicswo
as code owners
|
|
||
| const chats = useMemo(() => data?.pages.flatMap((page) => page.data) ?? [], [data]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Unconditional cache invalidation on mount: The useEffect with [userId, currentAccountId, queryClient] will invalidate the chats cache every time the ChatsProvider mounts (or when these deps change). This defeats the purpose of caching — every navigation to the chat list triggers a refetch. Consider whether a shorter staleTime or refetchOnWindowFocus/refetchOnMount on the query itself would be more appropriate and less heavy-handed.
| const avatarId = message.identity.id; | ||
| const avatarPath = message.identity?.icon; | ||
| const avatarPath = message.identity.icon; | ||
| const senderName = message.sender?.identity?.name || message.identity.name; |
There was a problem hiding this comment.
This uses || which will fall through on any falsy value (e.g., empty string ""). Consider using ?? (nullish coalescing) instead for a more precise fallback:
| const removeListener = addMessageListener((notification: ServerNotification) => { | ||
| if (notification.entity !== 'ChatMessage') return; | ||
|
|
||
| const message = notification.data as Message; |
| chatId, | ||
| } = useMessages(); | ||
|
|
||
| const shouldAutoScroll = (currentCount: number, previousCount: number): boolean => { |
There was a problem hiding this comment.
shouldAutoScroll and updatePreviousMessageCount are not memoized: These are plain functions defined inside the component body. While they're lightweight, for consistency with the rest of the component (which uses useCallback), consider wrapping them — or even better, inline the logic since they're only used once each.
scrollToNewestMessage has messages.length in its dependency array:
ts
const scrollToNewestMessage = useCallback(() => {
if (messages.length === 0) return;
// ...
}, [messages.length, handleScrollToIndexFailed]);
This recreates the callback every time messages.length changes, which is the exact moment it's called from the useEffect. This works but the dependency is arguably unnecessary since the early return (messages.length === 0) is a safety check. You could remove messages.length from the deps and rely on the useEffect guard (shouldAutoScroll) to prevent calling it when there are no messages.
| onScrollToIndexFailed={handleScrollToIndexFailed} | ||
| ListHeaderComponent={messagesFetchingNext ? <ActivityIndicator /> : null} | ||
| showsVerticalScrollIndicator={false} | ||
| maintainVisibleContentPosition={{ |
There was a problem hiding this comment.
This prop helps prevent the scroll position from jumping when new items are prepended. Removing it from an inverted FlatList could cause visual flickering when loading older messages (via handleLoadMore / infinite scroll). Was this intentional? If the custom scroll logic replaces this behavior, it should be documented.
| extraData={messages} | ||
| inverted | ||
| keyExtractor={(item) => item.id} | ||
| keyExtractor={(item, index) => item.id || `message-${index}`} |
There was a problem hiding this comment.
Using index as a fallback key in a list that supports pagination and live updates is risky — index-based keys can cause React to incorrectly reuse components when the data shifts. Under what circumstances would item.id be falsy? If it's from the SignalR notification, consider ensuring the ID is always present at the source.
| revision: number; | ||
| chat: Chat; | ||
| sender: Sender; | ||
| chat?: Chat; |
There was a problem hiding this comment.
This is a significant type relaxation. It means every consumer of Message now needs to handle the case where chat and sender are undefined. Only ChatMessage.tsx was updated in this PR — are there other consumers of Message that could break at runtime? A search for usages of message.chat and message.sender across the codebase is recommended before merging.
2 participants
