-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: tidy up Agentic Chat and Context accordions and behaviour #6735
base: main
Are you sure you want to change the base?
Conversation
73f72a8
to
4903df6
Compare
vscode/webviews/chat/cells/messageCell/assistant/SearchResults.tsx
Outdated
Show resolved
Hide resolved
vscode/webviews/chat/cells/messageCell/assistant/SearchResults.tsx
Outdated
Show resolved
Hide resolved
vscode/webviews/chat/cells/messageCell/assistant/SearchResults.tsx
Outdated
Show resolved
Hide resolved
<div className="tw-flex tw-flex-col tw-gap-2 tw-my-2 tw-border-t tw-border-t-muted tw-pt-4"> | ||
<div className="tw-text-sm tw-font-medium tw-text-foreground tw-flex tw-items-center tw-gap-3"> | ||
<DatabaseIcon size={14} strokeWidth={1.75} className="tw-text-muted-foreground" /> | ||
Context used | ||
</div> | ||
<ul className="tw-list-none tw-flex tw-flex-col tw-gap-1"> | ||
{contextItems?.map((item, i) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fully duplicated a few lines below. Can we clean that up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look resolved to me. I think this is really worth cleaning up since these can easily get out of sync if someone goes to make a change and only tests agentic or non-agentic chat.
vscode/webviews/components/codeSnippet/components/CodeExcerpt.module.css
Outdated
Show resolved
Hide resolved
9f57125
to
ccbb8c0
Compare
@abeatrix Can I get a review? |
const { boostedResults, nonBoostedResults } = useMemo(() => { | ||
if (!boostedRepo) { | ||
return { | ||
boostedResults: resultsToShow, | ||
nonBoostedResults: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this logic out of the templating language? It's quite surprising to me to see variable definitions + functions inside the template language, and I think it's actually a bug to call react hooks (like useMemo
) conditionally.
This looks really nice! The animations are ✨ I'll leave it to @abeatrix to hit approve since she's more familiar with the agentic chat domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started the build locally and Agentic Chat seems to work as expected, really like the animation on toggle:
The font color for the failed command makes it a bit hard to read though:
Do we need to update the storybook for the updated components? Specifically for AgenticContextCell as the ContextList now lives in it.
Also it's a bit hard to test the PR because it involves changes beyond the Agentic Chat
and context according (e.g. RepositorySelector, SearchResults, Welcome Footer etc). Is it possible to split them into separated PRs so that it'd be easier to test and review? 🙇♀️
steps => { | ||
// Only pass steps that should appear in the accordion | ||
const accordionSteps = steps.filter(step => step.type !== 'confirmation') | ||
statusUpdateCallback(accordionSteps) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
steps => { | |
// Only pass steps that should appear in the accordion | |
const accordionSteps = steps.filter(step => step.type !== 'confirmation') | |
statusUpdateCallback(accordionSteps) | |
}, | |
steps => statusUpdateCallback(steps), |
Do we need this? If yes, it might be better if we do the filtering in ProcessManager instead of the top-level?
isContextLoading={isContextLoading} | ||
processes={humanMessage?.processes ?? undefined} | ||
/> | ||
{humanMessage.agent && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{humanMessage.agent && ( | |
{!isSearchIntent && humanMessage.agent && ( |
I think @thenamankumar added the !isSearchIntent condition because of a feedback where Agentic Context is showing when we are displaying the Search result and the omnibox team doesn't want that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The single component is also used to show you're in a search (and be able to swap back and forth).
{!isSearchIntent && | ||
humanMessage.agent && | ||
isContextLoading && | ||
assistantMessage?.isLoading && <ApprovalCell vscodeAPI={vscodeAPI} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{!isSearchIntent && | |
humanMessage.agent && | |
isContextLoading && | |
assistantMessage?.isLoading && <ApprovalCell vscodeAPI={vscodeAPI} />} | |
{isContextLoading && | |
assistantMessage?.isLoading && <ApprovalCell vscodeAPI={vscodeAPI} />} |
Since we are nesting this component and we check for the humanMessage.agent at the top level already, we won't need this I assume?
However, we originally set the ApprovalCell component as an independent component instead of tying it to the AgenticContext to make the ApprovalCell reusable by other agents / non-agent processes. Is there any reason we want to tie it to Agentic Context?
Co-authored-by: Beatrix <[email protected]>
Co-authored-by: Beatrix <[email protected]>
This PR merges the multiple accordions we currently have into one UX.
Before
CleanShot.2025-01-27.at.13.32.08.mp4
After
CleanShot.2025-01-27.at.13.34.40.mp4
Test plan
This was tested locally, manually, in the following ways (where possible):
Accordion Behavior Tests
Context Panel Tests
Agentic Chat Integration
Cross-browser Testing
Edge Cases