feat(shadcn): add useExpression hook - #1415
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 48f0551 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
c2c9b1b to
a18ce27
Compare
thomasyuill-livekit
left a comment
thomasyuill-livekit
left a comment
There was a problem hiding this comment.
This is great @theomonnom
We should include a storybook example for testing and evaluate the behaviour with an agent that has expression enabled
And we should add tests for the hooks
I'm happy to contribute these to the PR if that's helpful
a18ce27 to
8a2ac0e
Compare
size-limit report 📦
|
8a2ac0e to
85a4e75
Compare
c0fecc8 to
654645a
Compare
654645a to
6471c3c
Compare
6471c3c to
026db07
Compare
026db07 to
94c3baf
Compare
theomonnom
changed the title
94c3baf to
0e2d557
Compare
0e2d557 to
b93668c
Compare
b93668c to
061a12f
Compare
thomasyuill-livekit
left a comment
thomasyuill-livekit
left a comment
There was a problem hiding this comment.
I believe we should add the useExpression hook to JS SDK and not Shadcn so it can be used by any React project.
our Shadcn Agents-UI library should be primarily concerned with presentation
since all the visualizers all ready support a color prop, I don't believe that Agents UI needs to be extended to support expressions.
We should not be including opionated "glue" code into our presentational components because that will make them brittle and less extensible. We should instead provide examples of how to glue these pieces together in our docs and starter templates
thomasyuill-livekit
left a comment
thomasyuill-livekit
left a comment
There was a problem hiding this comment.
Generally LGTM
just a few questions and suggestions
Also, looks like there's a linting error tripping up the tests
Would love @1egoman to give it a look too
| for (const segment of segments) { | ||
| const parsed = parseExpression(segment); | ||
| if (parsed) { | ||
| current = parsed; | ||
| speaker = segment.participantInfo.identity; | ||
| turnsSince = 0; | ||
| } else if (current && segment.participantInfo.identity === speaker) { | ||
| // only the expressing participant's own later turns age the mood | ||
| turnsSince++; | ||
| } | ||
| } |
There was a problem hiding this comment.
again couldn't we do this in reverse order?
| for (const segment of segments) { | |
| const parsed = parseExpression(segment); | |
| if (parsed) { | |
| current = parsed; | |
| speaker = segment.participantInfo.identity; | |
| turnsSince = 0; | |
| } else if (current && segment.participantInfo.identity === speaker) { | |
| // only the expressing participant's own later turns age the mood | |
| turnsSince++; | |
| } | |
| } | |
| turnsSince = 0; | |
| for (const segment of segments.reverse()) { | |
| turnsSince++; | |
| const parsed = parseExpression(segment); | |
| if (parsed) { | |
| current = parsed; | |
| speaker = segment.participantInfo.identity; | |
| continue; | |
| } | |
| } |
| let ticks = 0; | ||
| const id = setInterval(() => { | ||
| setSettled((previous) => { | ||
| const latest = segments.reduce<string | null>( |
There was a problem hiding this comment.
wouldn't it be faster to go backwards and find the first match?
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removes the `agent-expression` registry item and its color utilities as it was never part of the repo to being with (just a previous commit)
8965e24 to
66b7e6f
Compare
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense to me, just noticed a few nitpicks.
A higher level question - is useExpression a good name given how general "expression" is as a term (ie, somebody could think this relates to a mathematical expression, etc) ? Maybe useAgentExpression could be better?
| /** Published per transcript segment by Expressive Mode: the segment's leading delivery tag. */ | ||
| export const EXPRESSION_ATTRIBUTE = 'lk.expression'; |
There was a problem hiding this comment.
nitpick: Put this in here instead:
All the agent attributes constants are centralized in that enum.
|
|
||
| try { | ||
| const parsed = JSON.parse(raw) as { expression?: string; mood?: AgentMood }; | ||
| const expression = parsed.expression?.trim() || null; |
There was a problem hiding this comment.
nitpick: You likely want ?? here instead of ||:
| const expression = parsed.expression?.trim() || null; | |
| const expression = parsed.expression?.trim() ?? null; |
4 participants
No description provided.