Simplify and fix presence in ai-collab example app #3
Conversation
| @@ -51,7 +49,7 @@ export async function createAndInitializeContainer(): Promise< | |||
| export default function TasksListPage(): JSX.Element { | |||
| const [selectedTaskGroup, setSelectedTaskGroup] = useState<SharedTreeTaskGroup>(); | |||
| const [treeView, setTreeView] = useState<TreeView<typeof SharedTreeAppState>>(); | |||
| const [userPresenceGroup, setUserPresenceGroup] = useState<UserPresence>(); | |||
| const [presence, setPresence] = useState<IPresence>(); | |||
There was a problem hiding this comment.
Using IPresence "at the top" so we can leverage the events for updated data and for participants disconnecting, as well as the current participant's sessionId.
| ]); | ||
|
|
||
| // Detect when the page is closed | ||
| useEffect(() => { |
There was a problem hiding this comment.
Apparently unnecessary; when a tab closes, the attendeeDisconnected event fires correctly in other tabs with no extra code required.
| @@ -0,0 +1,17 @@ | |||
| /*! | |||
| * Copyright (c) Microsoft Corporation and contributors. All rights reserved. | |||
There was a problem hiding this comment.
Seeing error: /home/tong/workspace/FluidFramework/node_modules/.pnpm/next@14.2.8_@babel+core@7.25.7_react-dom@18.3.1_react@18.3.1/node_modules/next/dist/server/config.js:787
throw new Error(Configuring Next.js via '${(0, _path.basename)(nonJsPath)}' is not supported. Please replace the file with 'next.config.js' or 'next.config.mjs'.);
including next.config.ts in tsconfig.json...see how it goes
|
|
||
| import type { NextConfig } from "next/types"; | ||
|
|
||
| // We deliberately configure NextJS to not use React Strict Mode, so we don't get double-rendering of React components |
There was a problem hiding this comment.
Can you link to where this contract is documented?
|
|
||
| export interface User { | ||
| photo: string; | ||
| } | ||
|
|
||
| const statesSchema = { | ||
| onlineUsers: LatestMap<{ value: User }, `id-${string}`>(), | ||
| onlineUsers: Latest<User>({ photo: "" }), |
There was a problem hiding this comment.
Is empty string special for the photo property? Can we document why we're setting it that way?
| photoUrlsMap.set(update.key, update.value.value.photo); | ||
| setPhotoUrls([...photoUrlsMap.values()]); | ||
| userPresenceGroup.props.onlineUsers.events.on("updated", (update) => { | ||
| console.debug("Presence - 'updated' event", JSON.stringify(update)); |
There was a problem hiding this comment.
Do we want to keep the debug logs here? Seems reasonable to keep, just checking 🙂
| @@ -125,11 +107,11 @@ const UserPresenceGroup: React.FC<UserPresenceProps> = ({ | |||
| <Avatar alt="User Photo" src={photo} sx={{ width: 56, height: 56 }} /> | |||
| </StyledBadge> | |||
| ))} | |||
| {photoUrls.length > 4 && ( | |||
| {photoUrls.size > 4 && ( | |||
There was a problem hiding this comment.
Magic number 4: can we extract and document this?
There was a problem hiding this comment.
Sure. Adding comments here. It just looks prettier if there are more than 4 users. We will only show 4 users and a + remaining_number
chentong7
force-pushed
the
ai-tong
branch
2 times, most recently
from
73717f3 to
f45f4ea
Compare
|
|
||
| // Takes a presence object and returns the user presence object that contains the shared object states | ||
| export function buildUserPresence(presence: IPresence): UserPresence { | ||
| const states = presence.getStates(`name:user-avatar-states`, statesSchema); |
There was a problem hiding this comment.
Is name:user-avatar-states a special reserved string?
| if (isFirstRender.current) { | ||
| updateUserPresenceGroup().catch((error) => console.error(error)); | ||
| } |
There was a problem hiding this comment.
Nitpick: This pattern is fine, but it's more common to have a useEffect with an empty dependency array (will only run during first render) and then use a property of photoUrls's to determine whether it has been initialized, for example
isInitialized = photoUrls.get(presence.getMyself().sessionId) !== undefined
| @@ -20,6 +20,7 @@ | |||
| "paths": { | |||
| "@/*": ["./src/*"], | |||
| }, | |||
| "allowJs": true, | |||
There was a problem hiding this comment.
This probably got added back by accident right? @alexvy86
|
The relevant changes were already ported to microsoft#22850, we'll continue work there. |
This seems to fix the issues with presence we're having. A few callouts:
sessionIdprovided by the presence package.