Improve chat input #472
Improve chat input #472
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR significantly refactors the chat interface and LLM configuration management in Reor. Here's a focused review of the key changes:
- Introduces new hooks
useAgentConfiganduseLLMConfigsto centralize LLM and agent configuration management, though missing error handling and cleanup inuseAgentConfig - Potential race condition in
use-llm-configs.tswhen setting default LLM during config fetch - Inconsistent toast notification settings between global config and error handling in
App.tsx - Fixed width textarea in
ChatInput.tsx(600px) could cause responsive layout issues - Uncaught error potential in UI when LLM selection is missing in
ChatMessages.tsx
The changes improve code organization but introduce some reliability concerns that should be addressed before merging.
8 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| hideProgressBar={false} | ||
| closeOnClick | ||
| pauseOnHover | ||
| />{' '} |
There was a problem hiding this comment.
syntax: unnecessary trailing whitespace after ToastContainer closing tag
src/components/Chat/ChatInput.tsx
Outdated
| if (!e.shiftKey && e.key === 'Enter') { | ||
| e.preventDefault() | ||
| handleSubmitNewMessage() | ||
| } |
There was a problem hiding this comment.
logic: Submit message is now triggered even when input is empty, unlike previous version which checked userTextFieldInput
src/components/Chat/ChatInput.tsx
Outdated
| className={userTextFieldInput ? 'cursor-pointer' : ''} | ||
| /> | ||
| </div> | ||
| <div className=" flex w-full "> |
There was a problem hiding this comment.
syntax: Extra space in className string (' flex' and trailing space)
src/components/Chat/ChatInput.tsx
Outdated
| autoFocus | ||
| /> | ||
| <div className="mx-auto h-px w-[96%] bg-background/20" /> | ||
| <div className="flex h-10 flex-col items-center justify-between gap-2 py-2 md:flex-row md:gap-4"> |
There was a problem hiding this comment.
syntax: Extra space in gap-2 py-2
| const handleSubmitNewMessage = async () => { | ||
| if (userTextFieldInput) { | ||
| // this for v1 could just use the default agent config... | ||
| const agentConfigs = await window.electronStore.getAgentConfigs() | ||
| if (agentConfigs && agentConfigs.length > 0) { | ||
| handleNewChatMessage(userTextFieldInput, agentConfigs[0]) | ||
| } else { | ||
| handleNewChatMessage(userTextFieldInput) | ||
| if (!selectedLLM) { | ||
| throw new Error('Select an LLM.') | ||
| } |
There was a problem hiding this comment.
logic: Throwing an error directly will crash the UI. Should handle this gracefully with toast/alert instead.
| selectedLLM: string | undefined | ||
| setSelectedLLM: (value: string | undefined) => void |
There was a problem hiding this comment.
logic: selectedLLM type allows undefined but handleLLMChange assumes string. Add type guard or handle undefined case.
| const fetchAgentConfigs = async () => { | ||
| const agentConfigs = await window.electronStore.getAgentConfigs() | ||
| if (agentConfigs && agentConfigs.length > 0) { | ||
| setAgentConfig(agentConfigs[0]) | ||
| } else { | ||
| setAgentConfig(exampleAgents[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: Missing try/catch block around electronStore call. API calls should handle potential failures.
| useEffect(() => { | ||
| const fetchAgentConfigs = async () => { | ||
| const agentConfigs = await window.electronStore.getAgentConfigs() | ||
| if (agentConfigs && agentConfigs.length > 0) { | ||
| setAgentConfig(agentConfigs[0]) | ||
| } else { | ||
| setAgentConfig(exampleAgents[0]) | ||
| } | ||
| } | ||
| fetchAgentConfigs() | ||
| }, []) |
There was a problem hiding this comment.
style: Add cleanup function to cancel pending operations if component unmounts during fetch
| if (!storedDefaultLLM && fetchedLLMConfigs.length > 0) { | ||
| await window.llm.setDefaultLLM(fetchedLLMConfigs[0].modelName) | ||
| setDefaultLLM(fetchedLLMConfigs[0].modelName) | ||
| setDefaultLocalLLM(fetchedLLMConfigs[0].modelName) | ||
| } else { | ||
| setDefaultLLM(storedDefaultLLM) | ||
| setDefaultLocalLLM(storedDefaultLLM) | ||
| } |
There was a problem hiding this comment.
logic: Race condition possible between checking storedDefaultLLM and setting it. Should await the setDefaultLLM call before setting local state.
| @@ -4,20 +4,25 @@ import { LLMConfig } from 'electron/main/electron-store/storeConfig' | |||
|
|
|||
| const useLLMConfigs = () => { | |||
| const [llmConfigs, setLLMConfigs] = useState<LLMConfig[]>([]) | |||
| const [defaultLLM, setDefaultLLM] = useState<string>('') | |||
| const [defaultLLM, setDefaultLocalLLM] = useState<string>('') | |||
There was a problem hiding this comment.
style: Consider initializing defaultLLM as undefined instead of empty string to better represent unset state
No description provided.