-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Allow selecting an app in home chat input #2832
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,19 @@ | ||
| import { SendHorizontalIcon, StopCircleIcon } from "lucide-react"; | ||
| import { | ||
| SendHorizontalIcon, | ||
| StopCircleIcon, | ||
| FolderOpenIcon, | ||
| XIcon, | ||
| } from "lucide-react"; | ||
| import { | ||
| Tooltip, | ||
| TooltipTrigger, | ||
| TooltipContent, | ||
| } from "@/components/ui/tooltip"; | ||
|
|
||
| import { useSettings } from "@/hooks/useSettings"; | ||
| import { homeChatInputValueAtom } from "@/atoms/chatAtoms"; // Use a different atom for home input | ||
| import { homeChatInputValueAtom, homeSelectedAppAtom } from "@/atoms/chatAtoms"; | ||
| import { useAtom } from "jotai"; | ||
| import { useState } from "react"; | ||
| import { useStreamChat } from "@/hooks/useStreamChat"; | ||
| import { useAttachments } from "@/hooks/useAttachments"; | ||
| import { AttachmentsList } from "./AttachmentsList"; | ||
|
|
@@ -21,6 +27,8 @@ import { useChatModeToggle } from "@/hooks/useChatModeToggle"; | |
| import { useTypingPlaceholder } from "@/hooks/useTypingPlaceholder"; | ||
| import { AuxiliaryActionsMenu } from "./AuxiliaryActionsMenu"; | ||
| import { cn } from "@/lib/utils"; | ||
| import { useLoadApps } from "@/hooks/useLoadApps"; | ||
| import { AppSearchDialog } from "../AppSearchDialog"; | ||
|
|
||
| export function HomeChatInput({ | ||
| onSubmit, | ||
|
|
@@ -29,18 +37,24 @@ export function HomeChatInput({ | |
| }) { | ||
| const posthog = usePostHog(); | ||
| const [inputValue, setInputValue] = useAtom(homeChatInputValueAtom); | ||
| const [selectedApp, setSelectedApp] = useAtom(homeSelectedAppAtom); | ||
| const { settings } = useSettings(); | ||
| const { isStreaming } = useStreamChat({ | ||
| hasChatId: false, | ||
| }); // eslint-disable-line @typescript-eslint/no-unused-vars | ||
| useChatModeToggle(); | ||
|
|
||
| const [appSearchOpen, setAppSearchOpen] = useState(false); | ||
| const { apps } = useLoadApps(); | ||
|
|
||
| const typingText = useTypingPlaceholder([ | ||
| "an ecommerce store...", | ||
| "an information page...", | ||
| "a landing page...", | ||
| ]); | ||
| const placeholder = `Ask Dyad to build ${typingText ?? ""}`; | ||
| const placeholder = selectedApp | ||
| ? `Send a message to ${selectedApp.name}...` | ||
| : `Ask Dyad to build ${typingText ?? ""}`; | ||
|
|
||
| // Use the attachments hook | ||
| const { | ||
|
|
@@ -58,6 +72,14 @@ export function HomeChatInput({ | |
| cancelPendingFiles, | ||
| } = useAttachments(); | ||
|
|
||
| const handleSelectApp = (appId: number) => { | ||
| const app = apps.find((a) => a.id === appId); | ||
| if (app) { | ||
| setSelectedApp(app); | ||
| } | ||
| setAppSearchOpen(false); | ||
| }; | ||
|
|
||
| // Custom submit function that wraps the provided onSubmit | ||
| const handleCustomSubmit = () => { | ||
| if ( | ||
|
|
@@ -68,13 +90,18 @@ export function HomeChatInput({ | |
| return; | ||
| } | ||
|
|
||
| // Call the parent's onSubmit handler with attachments | ||
| onSubmit({ attachments }); | ||
| // Call the parent's onSubmit handler with attachments and selected app | ||
| onSubmit({ | ||
| attachments, | ||
| selectedApp: selectedApp ?? undefined, | ||
| }); | ||
|
|
||
| // Clear attachments as part of submission process | ||
| // Clear attachments and selected app as part of submission process | ||
| clearAttachments(); | ||
| setSelectedApp(null); | ||
|
Comment on lines
100
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| posthog.capture("chat:home_submit", { | ||
| chatMode: settings?.selectedChatMode, | ||
| existingApp: !!selectedApp, | ||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -162,6 +189,53 @@ export function HomeChatInput({ | |
| <div className="px-2 flex items-center justify-between pb-0.5 pt-0.5"> | ||
| <div className="flex items-center"> | ||
| <ChatInputControls showContextFilesPicker={false} /> | ||
| <Tooltip> | ||
| <TooltipTrigger | ||
| render={ | ||
| <button | ||
| onClick={() => setAppSearchOpen(true)} | ||
| className={cn( | ||
| "cursor-pointer px-2 py-1 ml-1.5 text-xs font-medium rounded-lg transition-colors flex items-center gap-1", | ||
| selectedApp | ||
| ? "bg-primary/10 text-primary hover:bg-primary/15" | ||
| : "text-foreground/80 hover:text-foreground hover:bg-muted/60", | ||
| )} | ||
| data-testid="home-app-selector" | ||
| /> | ||
| } | ||
| > | ||
| <FolderOpenIcon size={14} /> | ||
| <span className="truncate max-w-[150px]"> | ||
| {selectedApp ? selectedApp.name : "No app selected"} | ||
| </span> | ||
| {selectedApp && ( | ||
| <span | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setSelectedApp(null); | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.stopPropagation(); | ||
| setSelectedApp(null); | ||
| } | ||
| }} | ||
| className="hover:bg-primary/20 rounded-sm p-0.5 transition-colors" | ||
| aria-label="Deselect app" | ||
| data-testid="home-app-selector-clear" | ||
| > | ||
| <XIcon size={12} /> | ||
| </span> | ||
azizmejri1 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| )} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 HIGH | accessibility / interaction Nested The clear (X)
Found by 2/3 reviewers (Code Health Expert, UX Wizard). 💡 Suggestion: Restructure so the clear button is a sibling of the selector button, not a child. For example, wrap both in a flex <div className="flex items-center gap-1">
<Tooltip>
<TooltipTrigger render={<button ... />}>
<FolderOpenIcon size={14} />
<span className="truncate max-w-[150px]">{...}</span>
</TooltipTrigger>
<TooltipContent>{...}</TooltipContent>
</Tooltip>
{selectedApp && (
<button type="button" onClick={() => setSelectedApp(null)} ...>
<XIcon size={12} />
</button>
)}
</div> |
||
| </TooltipTrigger> | ||
|
Comment on lines
+193
to
+225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nested The clear/deselect button ( <button data-testid="home-app-selector"> <!-- outer trigger -->
...
<button data-testid="home-app-selector-clear"> <!-- NESTED – invalid HTML -->
<XIcon />
</button>
</button>Nesting interactive elements ( Fix: Lift the clear button out of <div className="flex items-center gap-1">
<Tooltip>
<TooltipTrigger render={<button onClick={() => setAppSearchOpen(true)} ... />}>
<FolderOpenIcon size={14} />
<span className="truncate max-w-[150px]">
{selectedApp ? selectedApp.name : "No app selected"}
</span>
</TooltipTrigger>
<TooltipContent>...</TooltipContent>
</Tooltip>
{selectedApp && (
<button
type="button"
onClick={() => setSelectedApp(null)}
...
data-testid="home-app-selector-clear"
>
<XIcon size={12} />
</button>
)}
</div>Prompt To Fix With AIThis is a comment left during a code review.
Path: src/components/chat/HomeChatInput.tsx
Line: 193-225
Comment:
**Nested `<button>` inside `<button>` — invalid HTML**
The clear/deselect button (`data-testid="home-app-selector-clear"`) is a child of `<TooltipTrigger render={<button .../>}>`. Because `@base-ui/react`'s `render` prop uses the provided element as the container and injects `TooltipTrigger`'s children into it, the rendered DOM ends up as:
```html
<button data-testid="home-app-selector"> <!-- outer trigger -->
...
<button data-testid="home-app-selector-clear"> <!-- NESTED – invalid HTML -->
<XIcon />
</button>
</button>
```
Nesting interactive elements (`<button>` inside `<button>`) is explicitly forbidden by the HTML spec. Browsers handle it inconsistently: some silently break the nesting and move the inner button outside, which means `e.stopPropagation()` in the clear handler may not behave as expected everywhere.
**Fix**: Lift the clear button out of `TooltipTrigger` and position it alongside (not inside) the trigger, e.g. by making the whole row a `flex` container with the `TooltipTrigger` button and the clear `<button>` as siblings. This preserves the same visual layout while producing valid HTML:
```jsx
<div className="flex items-center gap-1">
<Tooltip>
<TooltipTrigger render={<button onClick={() => setAppSearchOpen(true)} ... />}>
<FolderOpenIcon size={14} />
<span className="truncate max-w-[150px]">
{selectedApp ? selectedApp.name : "No app selected"}
</span>
</TooltipTrigger>
<TooltipContent>...</TooltipContent>
</Tooltip>
{selectedApp && (
<button
type="button"
onClick={() => setSelectedApp(null)}
...
data-testid="home-app-selector-clear"
>
<XIcon size={12} />
</button>
)}
</div>
```
How can I resolve this? If you propose a fix, please make it concise. |
||
| <TooltipContent> | ||
| {selectedApp | ||
| ? "Change selected app" | ||
| : "Select an existing app"} | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </div> | ||
|
|
||
| <AuxiliaryActionsMenu | ||
|
|
@@ -171,6 +245,19 @@ export function HomeChatInput({ | |
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <AppSearchDialog | ||
| open={appSearchOpen} | ||
| onOpenChange={setAppSearchOpen} | ||
| onSelectApp={handleSelectApp} | ||
azizmejri1 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| allApps={apps.map((a) => ({ | ||
| id: a.id, | ||
| name: a.name, | ||
| createdAt: a.createdAt, | ||
| matchedChatTitle: null, | ||
| matchedChatMessage: null, | ||
| }))} | ||
| /> | ||
azizmejri1 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </> | ||
| ); | ||
| } | ||
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.
🟡 MEDIUM | unnecessary complexity
handleSelectApp does a redundant ID round-trip through apps array
AppSearchDialogcallsonSelectApp(appId)with just the numeric ID, thenhandleSelectAppimmediately doesapps.find(a => a.id === appId)to recover the full app object. The dialog already has the fullAppSearchResult(id, name, createdAt) available at the call site.This pattern has two issues:
appshasn't finished loading or is stale,find()returnsundefinedand the selection is silently dropped — no feedback to the userHomeChatInputto calluseLoadApps()solely to support this lookup, creating an unnecessary dependencyFound by 2/3 reviewers (Correctness Expert, Code Health Expert).
💡 Suggestion: Change
onSelectAppto accept the full app object (or{ id, name }) instead of just the ID. This eliminates the lookup and the need foruseLoadApps()in this component.