Feat/tickets work 4#193
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements multiple UX improvements and bug fixes related to agent creation, chat interface, and authentication flow.
Key Changes:
- Made username a required field during agent creation with frontend validation
- Changed default free credits from $5 to $1
- Improved chat room title generation using AI instead of simple text truncation
- Enhanced chat input with dynamic single-line/multi-line layout based on content
- Streamlined wallet connection to show wallet options directly
- Fixed avatar removal from chat sidebar and improved scroll behavior
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/utils/character-names.ts | Updated default character creation to use blank fields instead of auto-generated names and avatars |
| lib/services/room-title.ts | Replaced simple text truncation with AI-powered title generation using GPT-4o-mini, added fallback logic for when AI fails |
| lib/privy-sync.ts | Changed default initial credits from $5.0 to $1.0 |
| lib/hooks/use-streaming-message.ts | Added completeCalled flag to prevent duplicate onComplete callback invocations |
| components/layout/chat-sidebar.tsx | Removed character avatar display and related imports from sidebar |
| components/chat/eliza-chat-interface.tsx | Implemented dynamic single-line/multi-line chat input with auto-resize, improved scroll area for better UX |
| components/chat/character-build-mode.tsx | Added username validation requiring non-empty value before saving |
| components/character-builder/character-form.tsx | Added asterisk to username label to indicate required field |
| app/login/page.tsx | Updated wallet login to use loginMethods parameter for direct wallet option display |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Auto-resize textarea when inputText changes | ||
| useEffect(() => { | ||
| if (textareaRef.current) { | ||
| textareaRef.current.style.height = "auto"; | ||
| textareaRef.current.style.height = Math.min(textareaRef.current.scrollHeight, 400) + "px"; | ||
| } |
There was a problem hiding this comment.
The auto-resize implementation directly manipulates the DOM by setting style.height. While functional, this creates a potential race condition if multiple rapid input changes occur. The height is set to "auto" and then immediately to scrollHeight, but if the component re-renders between these operations, it could cause visual jank. Consider using a debounced approach or ensuring the operations are batched in a single frame using requestAnimationFrame.
| // Auto-resize textarea when inputText changes | |
| useEffect(() => { | |
| if (textareaRef.current) { | |
| textareaRef.current.style.height = "auto"; | |
| textareaRef.current.style.height = Math.min(textareaRef.current.scrollHeight, 400) + "px"; | |
| } | |
| const resizeRafIdRef = useRef<number | null>(null); | |
| // Auto-resize textarea when inputText changes | |
| useEffect(() => { | |
| if (!textareaRef.current) { | |
| return; | |
| } | |
| // Cancel any pending resize to avoid race conditions/jank | |
| if (resizeRafIdRef.current !== null) { | |
| cancelAnimationFrame(resizeRafIdRef.current); | |
| } | |
| resizeRafIdRef.current = requestAnimationFrame(() => { | |
| const textarea = textareaRef.current; | |
| if (!textarea) return; | |
| textarea.style.height = "auto"; | |
| textarea.style.height = | |
| Math.min(textarea.scrollHeight, 400) + "px"; | |
| }); | |
| return () => { | |
| if (resizeRafIdRef.current !== null) { | |
| cancelAnimationFrame(resizeRafIdRef.current); | |
| resizeRafIdRef.current = null; | |
| } | |
| }; |
| import { cn } from "@/lib/utils"; | ||
| import { LockOnButton } from "@/components/brand"; | ||
| import { useChatStore } from "@/lib/stores/chat-store"; | ||
| import { SidebarBottomPanel } from "./sidebar-bottom-panel"; |
There was a problem hiding this comment.
The ElizaAvatar import is no longer used after removing the avatar display from the sidebar. This import should be removed.
| }, [inputText]); | ||
|
|
||
| // Track if input is expanded (multiline mode) | ||
| const isExpanded = inputText.includes("\n") || inputText.length > 80; |
There was a problem hiding this comment.
The condition for switching between single-line and multi-line input modes uses a hardcoded length threshold of 80 characters. This value appears in multiple places (lines 1112, 1439, 1554) which creates maintenance risk. Consider extracting this to a named constant at the top of the component to make it easier to adjust and maintain consistency.
| // Don't focus if clicking on a button or dropdown | ||
| const target = e.target as HTMLElement; | ||
| if (target.closest('button') || target.closest('[role="menu"]') || target.closest('[data-radix-popper-content-wrapper]')) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The selector check for preventing focus when clicking buttons uses a string-based query selector with hardcoded element types and attributes. The check for '[data-radix-popper-content-wrapper]' might not prevent focus if users click on nested elements within these components. Consider using stopPropagation on the actual button click handlers instead, or use a more robust event delegation approach.
| // Don't focus if clicking on a button or dropdown | |
| const target = e.target as HTMLElement; | |
| if (target.closest('button') || target.closest('[role="menu"]') || target.closest('[data-radix-popper-content-wrapper]')) { | |
| return; | |
| } | |
| // Only focus when clicking directly on the container, not on its children | |
| if (e.currentTarget !== e.target) { | |
| return; | |
| } |
| // Maintain focus when transitioning between layouts | ||
| useEffect(() => { | ||
| if (wasExpandedRef.current !== isExpanded) { | ||
| // Layout changed - restore focus to the appropriate input | ||
| requestAnimationFrame(() => { | ||
| if (isExpanded && textareaRef.current) { | ||
| textareaRef.current.focus(); | ||
| // Move cursor to end of text | ||
| const len = textareaRef.current.value.length; | ||
| textareaRef.current.setSelectionRange(len, len); | ||
| } else if (!isExpanded && inputRef.current) { | ||
| inputRef.current.focus(); | ||
| // Move cursor to end of text | ||
| const len = inputRef.current.value.length; | ||
| inputRef.current.setSelectionRange(len, len); | ||
| } | ||
| }); | ||
| } | ||
| wasExpandedRef.current = isExpanded; | ||
| }, [isExpanded]); |
There was a problem hiding this comment.
The transition between single-line input and multi-line textarea could cause accessibility issues. Screen readers may not properly announce the change when the input element type changes. Consider adding an aria-live region or maintaining focus announcement to ensure users with assistive technology are aware of the layout change.
| const words = message.trim().split(/\s+/).slice(0, 6); | ||
| if (words.length >= 3) { | ||
| return capitalizeFirst(words.slice(0, 5).join(" ")); |
There was a problem hiding this comment.
The fallback title generation logic at line 111 slices to 5 words but checks if words.length >= 3. This means messages with exactly 3 or 4 words will have all words included without ellipsis, but 5-word messages will also have all words without ellipsis (line 139 adds ellipsis only for messages with more than 5 words). Consider clarifying this logic or adjusting the threshold for consistency.
| const words = message.trim().split(/\s+/).slice(0, 6); | |
| if (words.length >= 3) { | |
| return capitalizeFirst(words.slice(0, 5).join(" ")); | |
| const words = message.trim().split(/\s+/); | |
| if (words.length >= 3) { | |
| const titleWords = words.slice(0, 5); | |
| const baseTitle = capitalizeFirst( | |
| titleWords.join(" ").replace(/[.!?]+$/, ""), | |
| ); | |
| return words.length > 5 ? baseTitle + "..." : baseTitle; |
| () => { | ||
| completeCalled = true; | ||
| onComplete?.(); | ||
| }, | ||
| ); | ||
| } catch (err) { | ||
| console.error("[Stream] Error processing final buffer:", err); | ||
| onError?.("Stream ended unexpectedly"); | ||
| } | ||
| } | ||
| // Always call onComplete when stream ends, if not already called | ||
| if (!completeCalled) { | ||
| onComplete?.(); | ||
| } |
There was a problem hiding this comment.
The onComplete callback can be called multiple times. While the new code adds a completeCalled flag to prevent duplicate calls, the flag is only set when wrapping onComplete for the final buffer processing (lines 196-199). However, when processSSEMessage is called in the regular message loop (line 236, which is outside the diff but calls onComplete directly), if a "done" event is received there, onComplete will be called without setting the completeCalled flag. This will cause onComplete to be called again at line 208. To fix this, the onComplete callback passed to processSSEMessage at line 236 should also use the same wrapper function.
| import Link from "next/link"; | ||
| import Image from "next/image"; | ||
| import { useRouter, usePathname } from "next/navigation"; | ||
| import { useRouter } from "next/navigation"; |
There was a problem hiding this comment.
The usePathname import is no longer used after removing the pathname-based logic for avatar selection. This import should be removed to keep the code clean.
| import { useRouter } from "next/navigation"; |
|
|
||
| const result = await generateText({ | ||
| model: gateway.languageModel("openai/gpt-4o-mini"), |
There was a problem hiding this comment.
The AI title generation uses a hardcoded model "openai/gpt-4o-mini". Consider making this configurable through environment variables to allow flexibility in model selection and to avoid hardcoding infrastructure dependencies.
| const result = await generateText({ | |
| model: gateway.languageModel("openai/gpt-4o-mini"), | |
| const roomTitleModel = | |
| process.env.ROOM_TITLE_MODEL || "openai/gpt-4o-mini"; | |
| const result = await generateText({ | |
| model: gateway.languageModel(roomTitleModel), |
| } | ||
|
|
||
| return finalTitle; | ||
| function capitalizeFirst(str: string): string { |
There was a problem hiding this comment.
The capitalizeFirst helper function doesn't handle empty strings safely. If an empty string is passed, charAt(0) will return an empty string and slice(1) will also return empty, but this could be made more explicit with a guard check to improve robustness.
| function capitalizeFirst(str: string): string { | |
| function capitalizeFirst(str: string): string { | |
| if (str.length === 0) { | |
| return str; | |
| } |
Comprehensive Code Review - PR #193Executive SummaryThis PR implements multiple UI/UX improvements with 221 additions and 51 deletions across 9 files. While the changes generally improve user experience, I've identified 2 Critical issues, 5 High severity issues, 8 Medium severity issues, and 6 Low severity issues that should be addressed before merging. 🔴 Critical Issues1. Username Validation Bug (character-build-mode.tsx:171-173)Issue: Validation doesn't check for empty strings, only undefined values. Current Code: if (\!character.username) {
toast.error("Username is required");
return;
}Fix: if (\!character.username?.trim()) {
toast.error("Username is required");
return;
}
// Add format validation
if (\!/^[a-zA-Z0-9_-]{3,20}$/.test(character.username)) {
toast.error("Username must be 3-20 characters (letters, numbers, _ and - only)");
return;
}2. Race Condition in Focus Management (eliza-chat-interface.tsx:1192-1210)Issue: Component could unmount between effect firing and Fix: Add cleanup function with 🟠 High Severity Issues3. Event Delegation Performance (eliza-chat-interface.tsx:1213-1225)Issue: Multiple 4. Textarea Auto-resize Layout Thrashing (eliza-chat-interface.tsx:1179-1185)Issue: Triggers 2 layout recalculations per keystroke. Should use ResizeObserver instead. 5. Avatar Removal Hurts UX (chat-sidebar.tsx:247-258)Issue: Users lose visual confirmation of selected character. 6. Double Callback Risk (use-streaming-message.ts:180-209)Issue: Completion callback might be called twice or inconsistently. Need safe wrapper function. 7. AI Title Generation - No Timeout (room-title.ts:67-75)Issue: Could hang indefinitely. No rate limiting, error budget, or caching implemented. 🟡 Medium Severity Issues
🔵 Low Severity Issues
📊 Summary by Severity
🎯 Recommended Action ItemsMust Fix:
Should Fix:
Nice to Have:
🌟 Positive Aspects
Great work on the UX improvements! The dynamic input expansion is particularly nice. Once the critical issues are addressed, this will be a solid enhancement to the user experience. |
https://linear.app/eliza-labs/issue/ELIZA-899/username-should-be-required-for-agent-during-agent-creation
https://linear.app/eliza-labs/issue/ELIZA-908/change-free-credits-from-dollar5-to-dollar1
https://linear.app/eliza-labs/issue/ELIZA-900/leave-agent-name-field-blank-during-agent-creation
https://linear.app/eliza-labs/issue/ELIZA-903/reduce-default-size-of-chat-box-to-one-line-dynamic-size-adjustment-as
https://linear.app/eliza-labs/issue/ELIZA-901/remove-agent-avatar-in-left-menu-in-chat
https://linear.app/eliza-labs/issue/ELIZA-910/scroll-should-work-on-whole-page
https://linear.app/eliza-labs/issue/ELIZA-909/connect-wallet-should-ideally-go-straight-to-wallet-options
https://linear.app/eliza-labs/issue/ELIZA-904/chat-summaries-dont-really-make-much-sense-can-we-improve