Skip to content

Conversation

@Abhijay007
Copy link
Collaborator

@Abhijay007 Abhijay007 commented Sep 2, 2025

Closes: #3747

Pull Request Description

This PR introduces session deletion functionality with a confirmation modal in the UI. It replaces the previous IPC approach in the existing PRs with a backend endpoint-based approach (as per Zane’s comment in this PR here: #2803 (comment))

Changes Made

  • Added a DELETE /sessions/{session_id}/delete endpoint with secure validation
  • Added a trash icon button in SessionListView with a confirmation modal for deletion
  • Updated logic to use session list validation (same as CLI) instead of the IPC approach

Preview

deleteSession

Copy link
Collaborator

@zanesq zanesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works nicely. Couple of nits below

Path Construction:
PathBuf::from(&session_info.path) assumes the path is always valid. If session_info.path is ever malformed or points outside the intended directory, this can be a security risk (path traversal). Can you add sanitizing/validating for the session path?

I noticed there is a line break in the delete confirmation and it flashes the delete confirmation again with delete button active after clicking, can you make it disappear after its done without showing again? (see video).

Screenshot 2025-09-02 at 3 11 14 PM
Kapture.2025-09-02.at.15.12.36.mp4

@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Sep 3, 2025

Hi @zanesq, I’ve resolved the requested changes. I removed PathBuf (a4a0954) for the path source and used the existing session::get_path() framework like in the other endpoints. This way we get the built-in framework validation, eliminate the path traversal risk, and maintain consistency across endpoints and the codebase.

I also updated the delete confirmation. However, could you please check again and let me know if it’s still happening? Maybe try deleting another chat to see if it reproduces. I’m not sure if it’s a title-related issue.

@Abhijay007 Abhijay007 requested a review from zanesq September 3, 2025 08:02
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
@zanesq
Copy link
Collaborator

zanesq commented Sep 3, 2025

Still seeing the line break in the confirmation dialog. The dialog still shows for a bit enabled when clicking it also but I guess its not a deal breaker if not an easy fix.

image

@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Sep 3, 2025

Still seeing the line break in the confirmation dialog. The dialog still shows for a bit enabled when clicking it also but I guess its not a deal breaker if not an easy fix.

image

Yeah, I am not sure why it's there or what the possible cause as I haven't used a line break here:

message={`Are you sure you want to delete the session "${sessionToDelete?.metadata.description || sessionToDelete?.id}"? This action cannot be undone.`}

Probably due to <DialogDescription/> in confirmationModal, let me look into that

@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Sep 3, 2025

Hi @zanesq , can you please try removing :

<DialogDescription className="whitespace-pre-wrap">{message}</DialogDescription>

The className="whitespace-pre-wrap" from <DialogDescription/> and see if the working, the thing is I tried that, but it was the same for me as it was (like there is no line break 😅), but I think it might solve this, so can you please try it once, and if that works, let me know. I will remove that

@zanesq zanesq merged commit 96d2b6f into block:main Sep 3, 2025
9 of 10 checks passed
michaelneale added a commit that referenced this pull request Sep 3, 2025
* main:
  Align Dynamic Task Interface with Recipe Interface (#4311)
  docs: copilot auth and mcp-ui links (#4497)
  docs: July and August 2025 Community All-Stars Update (#4501)
  remove clicking outside to close recipe warning (#4502)
  lower min width to 450 for small screens
  Convert recipe create and import forms to use tanstack form and zod schema validation (#4499)
  Repo CI: use a writable location for Goose home directory (#4500)
  feat: Add functionality to delete session in history list view (#4480)
  fix: recipe deeplink "+" characters and folder change (#4471)
  Add session to agents (#4216)
  fix: need to send errors to appropriate stream (#4491)
  Add Docker support for Goose in CI/CD pipelines (#4434)
  Add visual indicator while recipe loads (#4447)
  Disable chat input while extensions load (#4417)
  chore(release): release version 1.7.0 (#4391)
  fix double filtering (#4409)
  Rewrite the developer mcp using the rmcp sdk (#4297)
  docs: sessions reorg and conversation features (#4462)
katzdave added a commit that referenced this pull request Sep 4, 2025
* 'main' of github.com:block/goose:
  Align Dynamic Task Interface with Recipe Interface (#4311)
  docs: copilot auth and mcp-ui links (#4497)
  docs: July and August 2025 Community All-Stars Update (#4501)
  remove clicking outside to close recipe warning (#4502)
  lower min width to 450 for small screens
  Convert recipe create and import forms to use tanstack form and zod schema validation (#4499)
  Repo CI: use a writable location for Goose home directory (#4500)
  feat: Add functionality to delete session in history list view (#4480)
  fix: recipe deeplink "+" characters and folder change (#4471)
  Add session to agents (#4216)
  fix: need to send errors to appropriate stream (#4491)
  Add Docker support for Goose in CI/CD pipelines (#4434)
  Add visual indicator while recipe loads (#4447)
  Disable chat input while extensions load (#4417)
  chore(release): release version 1.7.0 (#4391)
This was referenced Sep 9, 2025
thebristolsound pushed a commit to thebristolsound/goose that referenced this pull request Sep 11, 2025
…#4480)

Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Co-authored-by: Zane Staggs <zane@squareup.com>
Signed-off-by: Matt Donovan <mattddonovan@protonmail.com>
HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 2025
…#4480)

Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
Co-authored-by: Zane Staggs <zane@squareup.com>
Signed-off-by: HikaruEgashira <hikaru-egashira@c-fo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding delete session option to Desktop UI

2 participants