fix(site/src/pages/AgentsPage): normalize chat menus to a kebab icon and matching items#26588
fix(site/src/pages/AgentsPage): normalize chat menus to a kebab icon and matching items#26588tracyjohnsonux wants to merge 5 commits into
Conversation
…and matching items The chat title menu used a horizontal ellipsis (meatball) and a different set of items than the sidebar chat-row menu. Designers asked to normalize both menus on the sidebar shape since the sidebar is the more recent design. - Switch the chat top bar trigger to the kebab icon (EllipsisVerticalIcon) so both menus use the same affordance. - Match the sidebar's items and order in the top bar: Pin/Unpin agent, Rename chat, Archive agent, Archive & delete workspace. Sentence case throughout. - Lift the rename-chat dialog state up to AgentsPageView so a single dialog instance is opened from either the sidebar row menu or the chat title menu via the outlet context. ChatsSidebar keeps internal-state fallback so existing stories and tests are unchanged. - Remove the standalone "Generate new title" item; the rename dialog's Generate button covers the same workflow and is what the sidebar already exposes.
Moves the agent actions kebab menu from the right-hand actions cluster into the title area, sitting immediately after the title text. The dropdown now aligns to its trigger's left edge so it reads as a title menu rather than a global action.
Extract the per-chat actions menu body into ChatActionsMenuItems so the sidebar's kebab/right-click menu and the chat top bar's kebab render the same JSX. The polymorphic Item/Separator props let the same component fill either a DropdownMenu or a ContextMenu. Both call sites now drive visibility off the same flags and handlers, so future changes can not drift between the two menus.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 8 findings (1 P2, 3 P3, 4 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel (15 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, Ging-ts, Ging-react, Gon, Leorio, Zoro, Robin, Meruem, Komugi, Kite). 1 P2, 3 P3, 4 Nit, 3 Note (3 dropped). Reviewed against 9b6dc4a..f914832. Round 2Churn guard: PROCEED. 8/8 open findings addressed in 19b5d74. Panel (7 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, Meruem). 0 new findings. All R1 fixes verified correct. Reviewed against fbf31f2..19b5d74. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean extraction of the shared ChatActionsMenuItems component, well-motivated by the design drift it prevents. The polymorphic Item/Separator slot pattern sits naturally on the existing DropdownMenu/ContextMenu shared styling. The controlled-or-uncontrolled rename dialog and the single-dialog-instance approach are both sound. The story coverage is thorough, especially the negative assertions in ArchivedWithUnarchive and ChildChatHidesPinAction.
Severity counts: 1 P2, 3 P3, 4 Nit.
The P2 is a race window where child chats can be pinned from the top bar because isChildChat waits on a secondary network fetch when the answer is already available from the primary query.
"The component already computes the right value at line 759:
const parentChatID = getParentChatID(chatQuery.data). The fix isisChildChat={parentChatID !== undefined}." (Hisoka)
The three P3s are: an orphan <Separator /> when no non-destructive items render, the top bar not threading isArchiving (undermining the 'cannot drift' goal on day one), and a mislabeled section comment.
site/src/pages/AgentsPage/AgentChatPageView.tsx:189
P3 [CRF-4] // Archive actions. now labels a block containing pin handlers, rename handler, isPinned, isChildChat, and isRegeneratingTitle, none of which are archive actions. Someone scanning this 80-prop interface for the pin handler will read "Archive actions", trust the grouping, and skip the block.
"Rename the section or split it. Something like
// Chat actions.if you want one group, or add a second// Pin & rename actions.break above the new props." (Leorio)
🤖
🤖 This review was automatically generated with Coder Agents.
- Derive isChildChat from getParentChatID on the primary chat query so Pin agent does not flicker on for child chats while the parent chat fetch is in flight (CRF-2, P2). The component already computes the same value used elsewhere. - Conditionally render the separator in ChatActionsMenuItems so it never appears alone above 'Archive agent' when no items render above it (CRF-1). - Thread isArchiving from the outlet context through ChatTopBar to the shared menu so the top bar's destructive items disable in lockstep with the sidebar's during an archive request (CRF-3). - Rename the misleading '// Archive actions.' section comment in AgentChatPageView to '// Chat actions.' since the block now also groups pin, rename, and regenerating-title state (CRF-4). - Drop doc comments that restate the type signature in ChatActionsMenuItems and AgentsPageView (CRF-5). - Rename handleOpenRenameDialog to handleOpenRenameDialogAction to match the Action suffix used by the other handlers in the block (CRF-6). - Use TypesGen.Chat throughout AgentsPageView instead of mixing a second named import (CRF-7). - Add negative assertions for the hidden Pin/Unpin label in PinAgentItem and UnpinAgentItem stories to mirror the pattern in ChildChatHidesPinAction and ArchivedWithUnarchive (CRF-8).
|
/coder-agents-review |
There was a problem hiding this comment.
All 8 findings from R1 were addressed in 19b5d74. The panel verified each fix against the code:
The P2 race window (CRF-2) is closed: isChildChat now derives from parentChatID, which reads parent_chat_id synchronously from the primary chat query. The isArchiving threading (CRF-3) is complete end-to-end from outlet context through to ChatActionsMenuItems. The orphan separator (CRF-1) is correctly gated. All nits addressed cleanly.
"Fixes address root causes, not symptoms. CRF-2 switches from
parentChat !== undefined(dependent on an async secondary query) toparentChatID !== undefined(derived synchronously from the primary chat query)." (Mafu-san)
No new findings in R2. The shared ChatActionsMenuItems component is structurally sound, the controlled/uncontrolled rename dialog pattern works correctly, and the story coverage is thorough with both positive and negative assertions across all conditional branches.
🤖 This review was automatically generated with Coder Agents.
The chat title menu used a horizontal ellipsis (meatball) and a different set of items than the sidebar chat-row menu. Designers asked to normalize both menus on the sidebar shape since the sidebar is the more recent design.
The chat top bar now uses the same
EllipsisVerticalIcon(kebab) as the sidebar row and exposes the same items in the same order: Pin/Unpin agent, Rename chat, Archive agent, and Archive & delete workspace. Labels are sentence case throughout. The standalone "Generate new title" item is removed because the rename dialog's Generate button covers the same workflow, which is what the sidebar already exposes. The kebab now sits inline with the title rather than off to the right, so it reads as a title menu instead of a global action.Both menus render from a single
ChatActionsMenuItemscomponent so they cannot drift in the future. The shared component is polymorphic overItemandSeparator, which lets the sidebar drive both its kebab (DropdownMenu) and its right-click context menu (ContextMenu) from the same JSX.Implementation notes
ChatActionsMenuItems.tsx: new shared component. Renders the Pin/Unpin, Rename, Archive, Archive & delete workspace, and Unarchive items off a flat set of flags (isArchived,isPinned,isChildChat,hasWorkspace,isArchiving) and zero-arg handlers, plus polymorphicItem/Separatorcomponents.ChatTopBar.tsx: replace the inlineEllipsisIcon+ duplicated dropdown body with the kebab trigger and a<ChatActionsMenuItems>call. Move the dropdown trigger into the title area so it sits next to the title text; switchDropdownMenuContenttoalign="start"so the menu opens flush with the trigger's left edge. Replace the oldonRegenerateTitle/isRegenerateTitleDisabledprops withonPinAgent,onUnpinAgent,onOpenRenameDialog,isPinned, andisChildChat.ChatTreeNode.tsx: delete the inlinerenderMenuItemshelper and call<ChatActionsMenuItems>from both theDropdownMenuContentand theContextMenuContent. Pin handlers are pre-bound tochat.id;onOpenRenameDialogis pre-bound to the chat object.AgentsPageView.tsx: lift the rename-chat dialog state up here and expose it throughAgentsOutletContext.onOpenRenameDialog, so the sidebar row menu and the chat top bar open the same dialog instance.ChatsSidebar.tsx: accept optional controlledchatPendingRename/onChatPendingRenameChangeprops with internal-state fallback (controlled-or-uncontrolled pattern). Existing stories and tests are untouched.AgentChatPage.tsx: consumerequestPinAgent,requestUnpinAgent, andonOpenRenameDialogfrom the outlet context, and pass newhandlePinAgentAction,handleUnpinAgentAction,handleOpenRenameDialog,isPinned, andisChildChatprops throughAgentChatPageViewto the top bar.ChatTopBar.stories.tsxreplaces theGenerateTitlestory withRenameChatItem,PinAgentItem,UnpinAgentItem, andChildChatHidesPinAction. Updated label expectations to sentence case across stories.AgentChatPage.stories.tsxupdated for the new "Archive agent" label.Decision log
ChatActionsMenuItemscomponent drives both menus so they cannot drift. Reviewers only need to read the menu items once.EllipsisVerticalIcon(kebab). The old meatball was the onlyEllipsisIconuse in this surface.requestRegenerateTitle,regeneratingTitleChatIds, the title spinner) is left in place because removing it expands the diff and the regenerate plumbing may be re-used; only the menu trigger is gone.RenameChatDialoginstances would have worked, but lifting the state toAgentsPageViewkeeps a single source of truth and lets the top bar open the dialog the user already knows.ChatsSidebarkeeps internal-state fallback so existing stories and tests do not need to thread state.isChildChat. Pin/Unpin is hidden for child chats to matchChatTreeNode, which only renders the Pin item when!isChildNode.Opened by Coder Agents on behalf of @tracyjohnsonux.