feat: update to Agent SDK #174
Conversation
| }, [containerRef, messages]); | ||
|
|
||
| // TODO(remove): debugging message rendering | ||
| console.log("messages", messages); |
| const newSettings = { ...config.settings }; | ||
| newSettings.agent_name = value; | ||
| setUserSettings(newSettings); | ||
| console.log("agent name changed to", value); |
| source={Track.Source.Microphone} | ||
| > | ||
| <AudioInputTile trackRef={localMicTrack} /> | ||
| {session.local.microphoneTrack ? ( |
There was a problem hiding this comment.
TIL - local tracks are available through the agent session object
| ? "hidden" | ||
| : "flex" | ||
| }`} | ||
| className={`flex gap-4 py-4 grow w-full selection:bg-${config.settings.theme_color}-900`} |
There was a problem hiding this comment.
string interpolation won't be picked up by tailwind
https://tailwindcss.com/docs/detecting-classes-in-source-files#dynamic-class-names
There was a problem hiding this comment.
Worth noting: I saw a few other instances of this in the change, but also quite a few instances of this just generally in the project. It might be worth doing a separate pass to clean this up across the board.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Sorry for the massive review, but I noticed a bunch of small things (and some broken behavior) you probably want to fix before merging.
| > | ||
| <div className="flex flex-col min-h-full justify-end"> | ||
| {messages.map((message, index, allMsg) => { | ||
| console.log("message", message); |
There was a problem hiding this comment.
(I think you probably want to get rid of this before merging)
| const [tokenFetchOptions, setTokenFetchOptions] = | ||
| useState<TokenSourceFetchOptions>({}); | ||
|
|
||
| // initialize token fetch options from initial values, which can come from config | ||
| useEffect(() => { | ||
| if (roomState === ConnectionState.Connected) { | ||
| localParticipant.setCameraEnabled(config.settings.inputs.camera); | ||
| localParticipant.setMicrophoneEnabled(config.settings.inputs.mic); | ||
| } | ||
| }, [config, localParticipant, roomState]); | ||
| setTokenFetchOptions({ | ||
| agentName: initialAgentOptions?.agentName, | ||
| agentMetadata: initialAgentOptions?.metadata, | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
thought: This technique you are using here of storing the agentName / agentMetadata in tokenFetchOptions like this could lead to some downstream problems, since tokenFetchOptions.agentName would start as undefined, but then if a user were to put something in the "agent name" textbox and get rid of it again, it would end up as an empty string.
Instead of doing it this way, it might be better to store the agent name / agent metadata separately and treat the the token fetch options as derived state built via a useMemo or similar.
Also I just want to call out since I think it's relevant: useSession doesn't need the tokenFetchOptions reference to be stable. It only keys on primative values within the object so if having it change every render simplifies things for some reason, that is perfectly fine to do.
| useEffect(() => { | ||
| // to debug room.incomingDataStreamManager | ||
| console.log("room handle", session.room); | ||
| }, [session.connectionState]); |
There was a problem hiding this comment.
(I think you probably want to get rid of this before merging)
| const localScreenTrack = session.room.localParticipant.getTrackPublication( | ||
| Track.Source.ScreenShare, |
There was a problem hiding this comment.
question: Not related to this change directly, but seeing this makes me wonder if there's a good reason to better expose the local participant screenshare track directly from useSession. It maybe could be a way to make this a bit more elegant...
|
|
||
| const chatTileContent = useMemo(() => { | ||
| if (voiceAssistant.agent) { | ||
| if (agent.internal.agentParticipant) { |
There was a problem hiding this comment.
nitpick: This I think can be agent.isConnected?
| if (agent.internal.agentParticipant) { | |
| if (agent.isConnected) { |
| const source = TokenSource.custom((options) => { | ||
| return { | ||
| serverUrl: url, | ||
| participantToken: token, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
nitpick: IMO, this probably makes more sense semantically as a TokenSource.literal:
| const source = TokenSource.custom((options) => { | |
| return { | |
| serverUrl: url, | |
| participantToken: token, | |
| }; | |
| }); | |
| const source = TokenSource.literal({ | |
| serverUrl: url, | |
| participantToken: token, | |
| }); |
| const [url, setUrl] = useState<string>(""); | ||
| const [token, setToken] = useState<string>(""); |
There was a problem hiding this comment.
question: I am not 100% sure here, but it looks like this is a behavior change? url / token will always be empty and not be defaulted to config.settings values now. Since I am not too familiar with this app, this might be intended, but it struck me as weird so I just wanted to call it out to make sure it was indeed intentional.
| type TokenRequest = { | ||
| room_name?: string; | ||
| participant_name?: string; | ||
| participant_identity?: string; | ||
| participant_metadata?: string; | ||
| participant_attributes?: Record<string, string>; | ||
| room_config?: ReturnType<RoomConfiguration["toJson"]>; |
There was a problem hiding this comment.
sugggestion: This could be TokenSourceRequestPayload instead.
| const roomName = request.room_name ?? request.roomName!; | ||
| const participantName = request.participant_name ?? request.participantName!; |
There was a problem hiding this comment.
suggestion: It looks like you got rid of the fallback to an auto generated default room name / participant name if one wasn't specified in the body. Was there a good reason for this? If not, it might be worth adding this back in since right now I think with an empty body (which per the token endpoint specification is allowed), this would result in unset jwt payload metadata.
| // const { shouldConnect, wsUrl, token, mode, connect, disconnect } = | ||
| // useConnection(); |
There was a problem hiding this comment.
nitpick: There's some dead code here you might want to get rid of.
No description provided.