Feature ai chat regenerate button #12794
Conversation
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
| deleteLastMessage(); | ||
| deleteLastMessage(); |
There was a problem hiding this comment.
Calling deleteLastMessage() twice creates code duplication. Consider creating a new method like deleteLastTwoMessages() to encapsulate this logic and improve maintainability.
| }); | ||
| if (chatMessage instanceof AiMessage) { | ||
| component.setOnRegenerate(comp -> { | ||
| if (!items.isEmpty() && items.getLast() == chatMessage && items.size() > 1) { |
There was a problem hiding this comment.
Direct access to the last element without checking if the list has sufficient elements could cause runtime exceptions. Should implement proper validation first.
There was a problem hiding this comment.
Reorder the conditions. I think, size check should come first and empty can be ommitted
| } | ||
|
|
||
| @FXML | ||
| public void initialize() { | ||
| uiChatHistory.setItems(aiChatLogic.getChatHistory()); | ||
| uiChatHistory.setRegenerateCallback(userPrompt -> { | ||
| // Remove the last two messages: first the AI response, then the corresponding user message |
There was a problem hiding this comment.
Comment duplicated in two places and simply restates what the code does without providing additional insight or reasoning about the implementation.
| if (items.size() > 1 && items.getLast() == chatMessage) { | ||
| ChatMessage previous = items.get(items.size() - 2); |
There was a problem hiding this comment.
The code doesn't follow fail-fast principle. The conditions should be checked separately with early returns to avoid deep nesting and improve readability.
src/main/java/org/jabref/gui/ai/components/aichat/chathistory/ChatHistoryComponent.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/aichat/chatmessage/ChatMessageComponent.java
Show resolved
Hide resolved
| }))); | ||
| for (ChatMessage chatMessage : items) { | ||
| ChatMessageComponent component = new ChatMessageComponent(chatMessage, comp -> { | ||
| items.remove(chatMessage); |
There was a problem hiding this comment.
I think you removed this code:
int index = vBox.getChildren().indexOf(chatMessageComponent);
items.remove(index);| }); | ||
| if (chatMessage instanceof AiMessage) { | ||
| component.setOnRegenerate(comp -> { | ||
| if (items.size() > 1 && items.getLast() == chatMessage) { |
There was a problem hiding this comment.
Is it possible to move out this code to AiChatComponent? So that ChatHistory component would be responsible only for history?
| if (previous instanceof UserMessage message) { | ||
| String userText = message.singleText(); | ||
| // Remove the last two messages: first the AI message, then the corresponding user message | ||
| items.removeLast(); |
There was a problem hiding this comment.
Shouldn't https://github.com/JabRef/jabref/pull/12794/files#diff-53b205bc2203a6a3cd923d087c923586b689bfc6f5a48f4f5951a91e0e816bf9R96 already remove messages everywhere?
| public void setOnRegenerate(Consumer<ChatMessageComponent> callback) { | ||
| this.onRegenerate.set(callback); | ||
| } | ||
|
|
||
| private void loadChatMessage() { |
There was a problem hiding this comment.
Can you also make that "Regenerate AI response" button is shown only if the message is an AiMessage or ErrorMessage?
|
Thanks for checking out AI features! But, I have to say that regeneration doesn't work as intended. It seems to generate a new answer to previous questions, but whole chat history is cleared |
Closes #12191
This PR adds a "Regenerate" button to the AI Chat tab in the entry editor.
Changes Made
chatmessage/ChatMessageComponent.fxmlchatmessage/ChatMessageComponent.javachatmessage/AiChatComponent.javachathistory/ChatHistoryComponent.javaSorry for the confusion, but I really don't know how I could add the "Regenerate AI response" to all properties. I only appended the sentence in following files.
resources/l10n/JabRef_en.propertiesJabRef_de.propertiesJabRef_fr.propertiesJabRef_pt_BR.propertiesHere's the screenshot.
Additional notes
Some unrelated tests in
mainalready fail before this PR. This PR does not touch any of those areas:ArgumentProcessorTest.convertBibtexToTableRefsAsBibPushToTeXworksTest.pushEntries()(macOS-specific behavior)ThemeManagerTest.liveReloadCssDataUrlMandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)