Add session permanent delete and click-drag multi-select#887
Open
Rhoahndur wants to merge 1 commit intohumanlayer:mainfrom
Open
Add session permanent delete and click-drag multi-select#887Rhoahndur wants to merge 1 commit intohumanlayer:mainfrom
Rhoahndur wants to merge 1 commit intohumanlayer:mainfrom
Conversation
- Add hard delete endpoint for sessions (any status) - Add bulk hard delete endpoint - Add delete confirmation dialog with keyboard shortcuts - Add action menu on session rows (archive/delete) - Add Backspace hotkey to delete focused/selected sessions - Add click-drag to select multiple sessions - Frontend http-client methods for delete operations
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to e6b08ea in 2 minutes and 50 seconds. Click for details.
- Reviewed
1601lines of code in9files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. hld/session/manager.go:1496
- Draft comment:
In the ContinueSession function the session config now uses req.GetQueryFromContentBlocks() instead of a raw query string. This is a good design for supporting mixed content; however, consider logging a warning if no text is found in the content blocks and the fallback query (c.Query) is used, so that edge cases (e.g. when only image blocks are provided) are tracked. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment assumes there's a fallback toc.Querywhen no text is found in content blocks, but I don't see this fallback in the visible code. The methodGetQueryFromContentBlocks()is called directly without any conditional logic checking if it returns empty. The comment also mentions tracking edge cases "when only image blocks are provided", but there's already a warning for image content at lines 1510-1514. Without seeing the implementation ofGetQueryFromContentBlocks(), I can't confirm if it has internal fallback logic toc.Query. The comment seems speculative about behavior that may not exist, or at least isn't visible in the changes. I might be missing the implementation details ofGetQueryFromContentBlocks()which could contain the fallback logic the comment refers to. The comment could be based on knowledge of that method's implementation that I don't have access to in this context. Even ifGetQueryFromContentBlocks()has internal fallback logic, the comment is speculative ("Consider logging...") rather than pointing to a definite issue. The code already has image content warnings. Without seeing clear evidence that this is a problem or that the fallback exists and needs tracking, this is more of a "nice to have" suggestion rather than an actionable code issue. The rules state not to make speculative comments. This comment is speculative and suggests a "nice to have" logging enhancement rather than identifying a clear code issue. There's already logging for image content, and the suggested fallback behavior isn't visible in the diff. This violates the rule against speculative comments.
2. hld/session/types.go:127
- Draft comment:
The GetQueryFromContentBlocks helper correctly concatenates text from all content blocks. It might be useful to add documentation (or tests) to clarify what happens when there are content blocks present but none with the type 'text' – i.e. ensuring the fallback to c.Query. This will make behavior explicit. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. humanlayer-wui/src/components/internal/SessionDetail/components/DeleteSessionDialog.tsx:21
- Draft comment:
The DeleteSessionDialog component cleanly implements the confirmation dialog with hotkeys. For improved accessibility, verify that underlying Dialog components set appropriate ARIA roles and labels (or add them explicitly) to assist screen readers. - Reason this comment was not posted:
Comment was on unchanged code.
4. humanlayer-wui/src/components/internal/SessionTable.tsx:312
- Draft comment:
The drag selection logic (using handleRowMouseDown and handleTableMouseMove) is implemented in a clear way by storing a potential drag start and then selecting a range. It would be good to ensure thorough testing of rapid mouse movements and to verify that all ref values (like isPotentialDragRef) are properly reset in every edge case. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure thorough testing and verify certain behaviors, which violates the rule against asking for confirmation or ensuring testing. It doesn't provide a specific suggestion or point out a specific issue.
5. humanlayer-wui/src/components/internal/SessionTable.tsx:800
- Draft comment:
The useHotkeys handler for the 'backspace' key to delete sessions is an intuitive approach. Consider adding further checks (for example, ensuring the event isn’t coming from within an input or editable element) to mitigate the risk of accidental deletions when the user might be editing text. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 25% vs. threshold = 50% The comment raises a valid concern about accidental deletions. However, looking at the implementation, there are already several protections in place: 1) The hotkey is disabled whenisInlineRenameOpenis true (line 822), 2) The hotkey is scoped to the table scope which may not be active when editing, 3) The libraryreact-hotkeys-hookmay have built-in protections. The comment suggests addingenableOnFormTags: falsewhich I see used in other hotkey handlers in this file (lines 723, 749, 766). This is actually a good suggestion - the backspace hotkey handler is missing this option that other handlers have. However, I need to consider if this is truly necessary given the existingisInlineRenameOpencheck. TheenableOnFormTags: falsewould provide defense-in-depth protection. The existingisInlineRenameOpencheck may already be sufficient since it explicitly checks if editing is happening. The comment is somewhat speculative ("to mitigate the risk") rather than pointing to a concrete bug. Also, I don't have full context on how the hotkey scoping system works - it's possible the scope-based protection is already sufficient. The comment doesn't provide evidence that there's an actual problem. While the existing checks may be sufficient, the comment does point out an inconsistency - other hotkey handlers in this file useenableOnFormTags: false(lines 723, 749, 766) but this new backspace handler doesn't. This suggests it might be a best practice that was overlooked. However, this is more of a "nice to have" defensive programming suggestion rather than a critical bug fix. The comment is also somewhat vague ("further checks") and doesn't specify exactly what should be added. The comment identifies a potential inconsistency (missingenableOnFormTags: false) but the existingisInlineRenameOpencheck likely provides adequate protection. The comment is speculative and doesn't demonstrate an actual bug. Given the rule to only keep comments with STRONG EVIDENCE of correctness, and that this is more of a defensive programming suggestion than a clear issue, I should delete this comment.
6. humanlayer-wui/src/components/internal/SessionTable.tsx:519
- Draft comment:
The mechanism capturing the 'g>e' hotkey to block the archive action triggered by a later 'e' press is clever. However, the 50ms threshold may be fragile across different devices or browsers. Consider testing on various platforms or making the threshold configurable. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. humanlayer-wui/src/lib/daemon/http-client.ts:448
- Draft comment:
In hardDeleteSession, the fetch call correctly checks the response status. To improve error diagnostics, consider parsing and logging the response body (if available) when response.ok is false, as this might provide more insight into why the deletion failed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This comment is suggesting a code quality improvement - better error diagnostics by parsing the response body. It's not pointing out a bug or something that will break. The comment uses "consider", which indicates it's a suggestion rather than identifying a required fix. According to the rules, comments should only be kept if there is "clearly a code change required" and should be "actionable and clear". While this is actionable and clear, it's not a required change - the code will work fine as-is. This falls into the category of a code quality refactor suggestion, which the rules say are "good! But only if they are actionable and clear." This one is actionable and clear, so it might be worth keeping. The comment is a reasonable suggestion for improving error diagnostics, and there is precedent in the codebase for this pattern (seelaunchDraftSession). This could genuinely help with debugging. The suggestion is actionable and specific. However, the word "consider" makes it sound optional rather than required, which might violate the spirit of "clearly a code change required." While the suggestion is reasonable and actionable, it's framed as optional ("consider") and is about improving diagnostics rather than fixing a bug or preventing an issue. The current implementation is functional and follows a common pattern. Without strong evidence that this specific endpoint returns useful error bodies that need to be logged, this is more of a "nice to have" than a required change. This comment should be deleted. While it's a reasonable code quality suggestion, it's framed as optional ("consider") and doesn't identify a required change. The current error handling is functional, and without evidence that this specific endpoint needs enhanced error parsing, this falls into the category of speculative improvements rather than clearly required changes.
8. humanlayer-wui/src/lib/daemon/http-client.ts:461
- Draft comment:
Similarly, in bulkHardDeleteSessions, if the fetch response is not OK, the error thrown only includes the status text. Enhancing the error message by including parsed details from the response body would aid debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This comment is about a newly added method in the diff, so it IS about changes. It's suggesting a code quality improvement - making error messages more informative by including response body details. This is actionable and clear. However, I need to check if this violates any rules. The rules say "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This seems to fit that criteria. It's not speculative, not about styling, not asking for confirmation, and it's a concrete suggestion. Looking at other methods in the file likehardDeleteSession(line 456), it has the same pattern of only including statusText. So this is a consistent pattern in the codebase, which might suggest the comment is nitpicky. But the comment is specifically about the NEW code being added, not existing code. The comment might be considered too minor or nitpicky since the existing codebase already has this same pattern (e.g.,hardDeleteSessionat line 456 also only includes statusText). If this pattern is acceptable elsewhere in the file, why should this new method be different? Also, the comment uses "would aid debugging" which is somewhat speculative about the value. While the pattern exists elsewhere, the comment is still about new code being added in this PR. The suggestion is actionable and clear - parse the response body and include it in the error. This is a legitimate code quality improvement. However, given that the same pattern exists inhardDeleteSession(also added in this diff at line 456), and the comment only targets one of the two methods, it seems inconsistent and potentially not important enough to warrant a comment. The comment is about new code and suggests an actionable improvement. However, it's inconsistent because the same issue exists inhardDeleteSession(line 456) which was also added in this diff, but wasn't commented on. This suggests the issue is either not important enough to warrant a comment, or the automated tool was inconsistent. Given the rule to only keep comments with STRONG EVIDENCE they are correct and important, and the inconsistency here, I should delete this comment.
Workflow ID: wflow_sNTw7wUgnbDMUx9L
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem(s) was I solving?
What user-facing changes did I ship?
How I implemented it
Backend:
DELETE /sessions/{id}/hard-deleteendpoint for single sessionPOST /sessions/hard-delete/bulkendpoint for batch deleteFrontend:
How to verify it
make check testpassesDescription for the changelog
Added permanent session deletion with confirmation dialog, action menu on rows, and click-drag multi-select for sessions.
Important
This PR adds permanent session deletion and click-drag multi-select functionality, with new backend endpoints and frontend updates including a confirmation dialog and hotkeys.
DELETE /sessions/{id}/hard-deleteandPOST /sessions/hard-delete/bulkendpoints insessions.gofor permanent session deletion.openapi.yamlto include new endpoints and request/response schemas.DeleteSessionDialogcomponent inDeleteSessionDialog.tsxfor delete confirmation with hotkeys (Cmd+Enter to confirm, Esc to cancel).SessionTable.tsxto include a dropdown menu with Archive and Delete options, and implemented click-drag multi-select functionality.SessionTable.tsx.http-client.tswith methodshardDeleteSessionandbulkHardDeleteSessionsfor API calls.DELETE_SESSION_DIALOGtoHOTKEY_SCOPESinscopes.ts.This description was created by
for e6b08ea. You can customize this summary. It will automatically update as commits are pushed.