Allow selecting an app in home chat input#2832
Conversation
|
@BugBot run |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by allowing users to choose an existing application when initiating a new chat from the home screen. This change moves beyond the previous default of always creating a new application, providing greater flexibility and control over where new conversations are housed. It integrates app selection directly into the chat input, streamlining the workflow for users who wish to continue work within an established application. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
Greptile SummaryThis PR adds the ability for users to select an existing app from the home chat input before sending a message, so the new chat is created inside that app instead of always creating a brand-new app. The change introduces a Key changes:
The main concern is the nested Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant HomeChatInput
participant homeSelectedAppAtom
participant AppSearchDialog
participant HomePage
participant IPC
User->>HomeChatInput: Clicks "No app selected" button
HomeChatInput->>AppSearchDialog: setAppSearchOpen(true) → mounts dialog
User->>AppSearchDialog: Selects an app
AppSearchDialog->>HomeChatInput: onSelectApp(appId)
HomeChatInput->>homeSelectedAppAtom: setSelectedApp(app)
HomeChatInput->>AppSearchDialog: setAppSearchOpen(false) → unmounts dialog
User->>HomeChatInput: Types message and submits
HomeChatInput->>HomePage: onSubmit({ attachments, selectedApp })
HomeChatInput->>homeSelectedAppAtom: setSelectedApp(null)
alt selectedApp is set (existing app flow)
HomePage->>IPC: ipc.chat.createChat(selectedApp.id)
IPC-->>HomePage: chatId (number)
else no selectedApp (new app flow)
HomePage->>IPC: ipc.app.createApp(...)
IPC-->>HomePage: { app, chatId }
end
HomePage->>IPC: streamMessage({ prompt, chatId, attachments })
HomePage->>HomePage: refreshApps / invalidateQueries
HomePage->>HomePage: selectChat({ chatId, appId })
Last reviewed commit: dfee809 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
bot
added
the
needs-human:review-issue
- Replace <span role="button"> with native <button> for accessibility in HomeChatInput - Add disableShortcut prop to AppSearchDialog to prevent duplicate Cmd/Ctrl+K listeners - Fix misleading error message when creating a chat in an existing app fails Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments addressed with code changes. Minor concerns remain around edge cases with the app selector UX but no blocking issues. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — principles were clear enough for all decisions. 🤖 Generated by Claude Code |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/chat/HomeChatInput.tsx">
<violation number="1" location="src/components/chat/HomeChatInput.tsx:242">
P2: Avoid conditionally rendering Radix Dialog components; use only the `open` prop to preserve exit animations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| </div> | ||
| </div> | ||
|
|
||
| {appSearchOpen && ( |
There was a problem hiding this comment.
P2: Avoid conditionally rendering Radix Dialog components; use only the open prop to preserve exit animations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/chat/HomeChatInput.tsx, line 242:
<comment>Avoid conditionally rendering Radix Dialog components; use only the `open` prop to preserve exit animations.</comment>
<file context>
@@ -246,18 +239,21 @@ export function HomeChatInput({
- matchedChatMessage: null,
- }))}
- />
+ {appSearchOpen && (
+ <AppSearchDialog
+ open={appSearchOpen}
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfee809705
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clearAttachments(); | ||
| setSelectedApp(null); |
There was a problem hiding this comment.
handleCustomSubmit clears homeSelectedAppAtom immediately, before the async submit flow in HomePage.handleSubmit completes. When ipc.chat.createChat(selectedApp.id) fails (for example, if the selected app was removed or IPC returns an error), the catch path in src/pages/home.tsx leaves the user on the home screen with their prompt still present but no selected app, so a retry silently falls back to creating a new app instead of targeting the originally chosen one.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <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 && ( | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| 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} /> | ||
| </button> | ||
| )} | ||
| </TooltipTrigger> |
There was a problem hiding this 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:
<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:
<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 AI
This 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.
| > | ||
| <XIcon size={12} /> | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
🔴 HIGH | accessibility / interaction
Nested <button> elements are invalid HTML
The clear (X) <button> on line 212 is nested inside the outer <button> rendered by TooltipTrigger (line 193). Nested interactive elements are invalid per the HTML spec and cause:
- Unpredictable click handling: Clicking the X may fire both the inner click (clear) and the outer click (open dialog) simultaneously in some browsers
- Broken keyboard navigation: Screen readers and keyboard users cannot reliably interact with the inner button
- DOM parsing issues: Browsers may restructure the DOM in unexpected ways when encountering nested buttons
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> — the tooltip wraps just the selector button, and the X button sits alongside it:
<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>| setSelectedApp(app); | ||
| } | ||
| setAppSearchOpen(false); | ||
| }; |
There was a problem hiding this comment.
🟡 MEDIUM | unnecessary complexity
handleSelectApp does a redundant ID round-trip through apps array
AppSearchDialog calls onSelectApp(appId) with just the numeric ID, then handleSelectApp immediately does apps.find(a => a.id === appId) to recover the full app object. The dialog already has the full AppSearchResult (id, name, createdAt) available at the call site.
This pattern has two issues:
- If
appshasn't finished loading or is stale,find()returnsundefinedand the selection is silently dropped — no feedback to the user - It requires
HomeChatInputto calluseLoadApps()solely to support this lookup, creating an unnecessary dependency
Found by 2/3 reviewers (Correctness Expert, Code Health Expert).
💡 Suggestion: Change onSelectApp to accept the full app object (or { id, name }) instead of just the ID. This eliminates the lookup and the need for useLoadApps() in this component.
🎭 Playwright Test Results❌ Some tests failed
Summary: 741 passed, 1 failed, 13 flaky, 240 skipped Failed Tests🪟 Windows
|
2 participants
Uh oh!
There was an error while loading. Please reload this page.