-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve user messages at choice prompts in history (fixes #7316) #7317
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
Conversation
- Added local message history buffer in usePromptHistory hook to store messages immediately upon sending - Modified ChatTextArea to save messages to local history before clearing input field - Ensured messages typed at follow-up question prompts are preserved and accessible via up-arrow navigation - Added deduplication logic to prevent duplicate entries when messages are confirmed by backend This fix ensures that user messages are never lost, even if typed at choice prompts or if backend processing fails.
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| }, []) | ||
|
|
||
| // Add a message to local sent history | ||
| const addToLocalHistory = useCallback((message: string) => { |
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.
Missing test coverage for this new functionality. Could we add tests to verify that messages are properly saved to local history when Enter is pressed at choice prompts? The existing tests don't cover the addToLocalHistory function or the local message preservation logic.
| const [tempInput, setTempInput] = useState("") | ||
| const [promptHistory, setPromptHistory] = useState<string[]>([]) | ||
| // Local sent messages that haven't been confirmed by backend yet | ||
| const [localSentMessages, setLocalSentMessages] = useState<string[]>([]) |
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.
Potential memory concern here. The localSentMessages array grows up to MAX_PROMPT_HISTORY_SIZE but is only cleared when clineMessages becomes empty. In a long-running session, could this accumulate duplicates? Consider adding deduplication when adding to localSentMessages or clearing confirmed messages from the local buffer.
|
|
||
| // Add to local history before sending | ||
| if (inputValue.trim()) { | ||
| addToLocalHistory(inputValue.trim()) |
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.
Is there a potential race condition between adding to local history and the actual send? If onSend() fails or is delayed, the message is already in local history. Is this intentional to preserve the message even on send failure, or should we only add after confirming the send was initiated?
| .map((message) => message.text!) | ||
|
|
||
| // If we have conversation messages, use those (newest first when navigating up) | ||
| // Combine conversation prompts with local sent messages (deduplicated) |
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.
This deduplication logic (lines 52-71) could be extracted into a utility function for better readability and potential reuse. Something like deduplicateAndMergePrompts(conversationPrompts, localSentMessages) would make the intent clearer.
| ) => boolean | ||
| resetHistoryNavigation: () => void | ||
| resetOnInputChange: () => void | ||
| addToLocalHistory: (message: string) => void |
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.
Consider adding JSDoc comments for the new addToLocalHistory function and localSentMessages state to explain their purpose and when they're used. This would help future maintainers understand why we need this local buffer for fixing the UX issue.
|
Closing in favor of #7394 |
This PR fixes issue #7316 where messages typed at follow-up question prompts were disappearing from history.
Problem
When users typed messages at choice prompts (follow-up questions), the messages would disappear and not be preserved in the chat history or accessible via up-arrow navigation. This was particularly problematic if the backend processing failed or if users navigated away.
Solution
usePromptHistoryhook that stores messages immediately upon sendingChatTextAreato save messages to local history before clearing the input fieldChanges Made
webview-ui/src/components/chat/hooks/usePromptHistory.ts:localSentMessagesstate to track messages before backend confirmationfilteredPromptHistoryto combine backend-confirmed messages with local sent messagesaddToLocalHistoryfunction to preserve messages locallywebview-ui/src/components/chat/ChatTextArea.tsx:addToLocalHistoryfrom the hookhandleKeyDownto save messages to local history before sendingTesting
Fixes #7316
Important
Fixes issue #7316 by preserving user messages at choice prompts in history, allowing access via up-arrow navigation and preventing duplicates.
usePromptHistorybefore backend confirmation.ChatTextArea.tsx:addToLocalHistoryto save messages before clearing input.handleKeyDownto ensure messages are preserved when Enter is pressed.usePromptHistory.ts:localSentMessagesstate to track unconfirmed messages.filteredPromptHistory.This description was created by
for 7edeb0d. You can customize this summary. It will automatically update as commits are pushed.