-
Notifications
You must be signed in to change notification settings - Fork 453
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
Final chat fixes #479
Final chat fixes #479
Conversation
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.
PR Summary
This PR streamlines the chat interface by simplifying agent configurations, fixing UI layout issues, and improving component organization.
- Simplified default agent prompt template in
src/components/Chat/ChatConfigComponents/exampleAgents.ts
by removing tool references and streamlining query format - Added dynamic font sizing in
src/components/Chat/MessageComponents/UserMessage.tsx
based on message length (text-lg/xl/2xl) - Added new chat button to window controls via
onNewChat
prop insrc/components/ui/window-controls.tsx
- Fixed z-index and positioning issues for hover cards in
src/components/Chat/MessageComponents/ChatSources.tsx
- Removed automatic chatbot visibility control from
ContentContext
, moving it to a more appropriate location
10 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -64,8 +64,8 @@ const ChatSources: React.FC<ChatSourcesProps> = ({ contextItems }) => { | |||
</CardDescription> | |||
</Card> | |||
</HoverCardTrigger> | |||
<HoverCardContent className="max-h-[60vh] w-80 overflow-y-auto"> | |||
<div className="p-2"> | |||
<HoverCardContent className="z-[100] max-h-[60vh] w-80 overflow-y-auto" sideOffset={5}> |
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.
style: z-[100] may not be high enough if other overlays or modals have higher z-indices. Consider using a z-index management system.
<HoverCardContent className="max-h-[60vh] w-80 overflow-y-auto"> | ||
<div className="p-2"> | ||
<HoverCardContent className="z-[100] max-h-[60vh] w-80 overflow-y-auto" sideOffset={5}> | ||
<div className=""> |
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.
style: removing padding without replacement may cause content to touch edges of container
if (messageLength > 100) return 'text-lg' | ||
if (messageLength > 50) return 'text-xl' | ||
return 'text-2xl' |
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.
style: Abrupt font size transitions at 50/100 chars may cause visual jarring. Consider adding more gradual breakpoints or using a scaling formula
<ReactMarkdown | ||
rehypePlugins={[rehypeRaw]} | ||
className={`max-w-[95%] break-words font-bold text-neutral-400 ${getFontSizeClass()}`} | ||
> |
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.
logic: ReactMarkdown with rehypeRaw enabled on user messages could pose XSS risks if message content is not properly sanitized
{showChatbot && ( | ||
<ResizablePanel defaultSize={currentlyOpenFilePath && showEditor ? 35 : 100}> |
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.
style: Consider adding a loading state while openNewChat is being executed to prevent multiple rapid clicks
@@ -85,7 +85,11 @@ const MainPageContent: React.FC = () => { | |||
{showChatbot && ( | |||
<ResizablePanel defaultSize={currentlyOpenFilePath && showEditor ? 35 : 100}> |
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.
logic: defaultSize calculation could cause layout jump if currentlyOpenFilePath or showEditor change after initial render
{onNewChat && ( | ||
<button | ||
type="button" | ||
onClick={onNewChat} | ||
className="cursor-pointer bg-transparent p-1.5 transition-colors" | ||
title="New Chat" | ||
> | ||
<Plus className="size-3 text-neutral-400 hover:text-neutral-300" /> | ||
</button> | ||
)} |
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.
logic: The new button should have an aria-label attribute for screen reader accessibility, similar to the other window control buttons
if (chatMetadata) { | ||
setShowChatbot(true) | ||
openNewChat(pathOrChatID) | ||
} else { |
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.
logic: Removing automatic chatbot visibility toggle here changes UX - ensure chat visibility is handled appropriately in ChatContext or the component level
No description provided.