Conversation
Update chat history deletion to track failed deletion individually on a chat-by-chat level. Fix long chat name truncation in history view.
![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 introduces undo functionality for chat history deletion by using delayed deletion tasks, visual progress feedback, and updated UI components to support multiple rapid deletion actions.
- macos/Onit/Data/Model/Model+History.swift: Implements async deletion with a delay, cancellation checks, and separates queues for normal and failed deletions.
- macos/Onit/Data/Model/OnitModel.swift: Defines deletion state types and manages multiple deletion tasks with a 3-second undo window.
- macos/Onit/UI/History/HistoryDeleteNotification.swift: Adds an animated undo notification with a 60fps timer and decaying progress bar.
- macos/Onit/UI/History/HistoryRowView.swift: Adjusts deletion button behavior with improved text truncation.
- macos/Onit/UI/Components/ProgressBar.swift: Provides a reusable progress component for undo countdown visuals.
8 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| func deleteChat(chat: Chat) async { | ||
| do { | ||
| deleteChatFailed = false | ||
| await removeChatFromDeleteFailedQueue(chatId: chat.id, waitSeconds: 0) | ||
|
|
||
| try await Task.sleep(for: .seconds(deleteChatDurationSeconds)) | ||
| try Task.checkCancellation() | ||
|
|
||
| removeChatFromDeleteQueue(chatId: chat.id) | ||
|
|
||
| container.mainContext.delete(chat) | ||
| try container.mainContext.save() | ||
| } catch { |
There was a problem hiding this comment.
logic: Cancellation in deleteChat is always treated as a failure (adding the chat to the failed queue). Consider distinguishing CancellationError from other errors to avoid mislabeling intentional undos as failures.
| GeometryReader { geometry in | ||
| HStack {} | ||
| .frame(maxHeight: .infinity) | ||
| .frame(width: geometry.size.width * progressPercentage) | ||
| .background(.white) | ||
| } |
There was a problem hiding this comment.
style: Consider using a Rectangle() instead of an empty HStack for the progress fill for better semantic clarity.
…rrors, preventing Onit from mistaking the undoing of chat history deletion as being a legitimate error. Update empty HStacks in ProgressBar to Rectangle for better semantic clarity.
timlenardo
left a comment
timlenardo
left a comment
There was a problem hiding this comment.
Requesting changes due to the issues I've mentioned in Slack:
- If you’re in an active chat, and delete that chat from history, the chat still stays open in the OnitPanel. I found that weird. I think it should probably disappear and start a new chat.
- If I delete a chat, it’s still accessible in history via the up and down arrows.
- I saw one weird case where I deleted a chat from history, then relaunched the app. The chat I deleted was still in my history, but a different chat had lost its responses. I haven't been able to reproduce this, but I do think there's something funky going on with the deletion that needs to be debugged.
I've looked at the code, and the delete() function seems correct, so I suspect the issues are coming from the timeout, and storing the Chat to be deleted in chatQueuedForDeletion. Is it possible this becomes the wrong Chat? chatQueuedForDeletion is only a single Chat, so what happens if you delete a bunch simultaneously? Or, is it possible that the HistoryRowView gets recycled and the wrong Chat gets selected? Either way, can you test this extensively to see if you can trigger these edge cases? Some of my testing is coming back fishy 🤨
2 participants
Task
Designs
Add ability to undo history deletion.
Notes