- Notifications
You must be signed in to change notification settings - Fork905
feat: embed chat ui in the task sidebar#18216
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
code-asher left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I only looked it over and have not run it, but it looks good to me. It was hard for me to tell which parts moved and which parts changed when they are in one commit, so I may have reviewed some of the moved code, you can ignore those comments if you like. 😅
site/src/pages/TaskPage/TaskApps.tsx Outdated
| <main className="flex-1 flex flex-col"> | ||
| <div className="border-0 border-b border-border border-solid w-full p-1 flex gap-2"> | ||
| {embeddedApps | ||
| .filter((app) => !app.external) |
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.
embeddedApps is already filtered so we could remove this.
site/src/pages/TaskPage/TaskApps.tsx Outdated
| if (app.external) { | ||
| return; | ||
| } |
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.
app.external would always be false sinceembeddedApps is filtered.
site/src/pages/TaskPage/TaskApps.tsx Outdated
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent> | ||
| {externalApps | ||
| .filter((app) => app.external) |
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.
externalApps is already filtered so we could remove this.
| }); | ||
| return ( | ||
| <iframe |
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.
Is it possible we might want to eventually use a static reference for the iframe? Wondering if there might be cases where the iframe is recreated and the user loses their position in the iframe, since React seems to re-render so much.
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.
Right now it is working as expected but we definitely can improve it if that is the case.
| .sort( | ||
| (a, b) => | ||
| new Date(b.created_at).getTime() - new Date(a.created_at).getTime(), | ||
| ); |
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.
Formatters are wild sometimes lol
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.
Since this is all contained in the task page, just stamping this.
13449b9 intomainUh oh!
There was an error while loading.Please reload this page.
Demo:
Does not include: