Skip to content

feat: Add modal for critical actions #7004

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Add modal for critical actions #7004

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
426ab2d
Add destructive props for Delete workspace action
tristantr Oct 30, 2025
7c8dee2
Add confirmation modals when deleting/archiving a workspace
tristantr Oct 30, 2025
4b7b8ba
Make one loading state per modal
tristantr Oct 30, 2025
484350c
Handle error on modals
tristantr Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 68 additions & 15 deletions frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,14 @@
} from '$lib/components/workspaceSettings/DucklakeSettings.svelte'
import { AIMode } from '$lib/components/copilot/chat/AIChatManager.svelte'
import UnsavedConfirmationModal from '$lib/components/common/confirmationModal/UnsavedConfirmationModal.svelte'
import ConfirmationModal from '$lib/components/common/confirmationModal/ConfirmationModal.svelte'
import TextInput from '$lib/components/text_input/TextInput.svelte'
let archiveModalOpen = $state(false)
let deleteModalOpen = $state(false)
let isProcessingArchive = $state(false)
let isProcessingDelete = $state(false)
let slackInitialPath: string = $state('')
let slackScriptPath: string = $state('')
let teamsInitialPath: string = $state('')
Expand Down Expand Up @@ -742,30 +748,18 @@
disabled={$workspaceStore === 'admins' || $workspaceStore === 'starter'}
unifiedSize="md"
btnClasses="mt-2"
on:click={async () => {
await WorkspaceService.archiveWorkspace({ workspace: $workspaceStore ?? '' })
sendUserToast(`Archived workspace ${$workspaceStore}`)
workspaceStore.set(undefined)
usersWorkspaceStore.set(undefined)
goto('/user/workspaces')
}}
on:click={() => (archiveModalOpen = true)}
>
Archive workspace
</Button>

{#if $superadmin}
<Button
color="red"
destructive
disabled={$workspaceStore === 'admins' || $workspaceStore === 'starter'}
size="sm"
btnClasses="mt-2"
on:click={async () => {
await WorkspaceService.deleteWorkspace({ workspace: $workspaceStore ?? '' })
sendUserToast(`Deleted workspace ${$workspaceStore}`)
workspaceStore.set(undefined)
usersWorkspaceStore.set(undefined)
goto('/user/workspaces')
}}
on:click={() => (deleteModalOpen = true)}
>
Delete workspace (superadmin)
</Button>
Expand Down Expand Up @@ -1050,5 +1044,64 @@
tabMode={true}
/>

<ConfirmationModal
open={archiveModalOpen}
title="Archive workspace"
confirmationText="Archive"
type="danger"
loading={isProcessingArchive}
onConfirmed={async () => {
isProcessingArchive = true
try {
await WorkspaceService.archiveWorkspace({ workspace: $workspaceStore ?? '' })
Copy link
Contributor

@claude claude bot Oct 30, 2025

Choose a reason for hiding this comment

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

Critical: Missing error handling

If WorkspaceService.archiveWorkspace() throws an error, the user won't see a helpful error message, and the modal will close even though the operation failed.

Recommendation:

try {
	await WorkspaceService.archiveWorkspace({ workspace: $workspaceStore ?? '' })
	sendUserToast(`Archived workspace ${$workspaceStore}`)
	workspaceStore.set(undefined)
	usersWorkspaceStore.set(undefined)
	goto('/user/workspaces')
	archiveModalOpen = false
} catch (error) {
	sendUserToast(`Failed to archive workspace: ${error.message}`, 'error')
	// Keep modal open so user can retry
} finally {
	isProcessingArchive = false
}

This way, on error:

  1. User sees a clear error message
  2. Modal stays open (they can retry or cancel)
  3. Loading state is cleared

Only close the modal on success.

Copy link
Contributor

@rubenfiszel rubenfiszel Oct 31, 2025

Choose a reason for hiding this comment

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

@tristantr claude is correct

sendUserToast(`Archived workspace ${$workspaceStore}`)
workspaceStore.set(undefined)
usersWorkspaceStore.set(undefined)
goto('/user/workspaces')
archiveModalOpen = false
} catch (error) {
sendUserToast(`Failed to archive workspace: ${error}`, true)
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Oct 31, 2025

Choose a reason for hiding this comment

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

In the catch block for archiving, the error is directly interpolated in the toast. Consider checking if error is an instance of Error and using error.message to avoid unexpected output.

Suggested change
sendUserToast(`Failed to archive workspace: ${error}`, true)
sendUserToast(`Failed to archive workspace: ${error instanceof Error ? error.message : error}`, true)

} finally {
isProcessingArchive = false
}
}}
onCanceled={() => (archiveModalOpen = false)}
>
<span>
Are you sure you want to archive workspace <b class='text-emphasis font-semibold'>{$workspaceStore}</b>? This action can be reversed by a superadmin.
</span>
</ConfirmationModal>

<ConfirmationModal
open={deleteModalOpen}
title="Delete workspace permanently"
confirmationText="Delete"
type="danger"
loading={isProcessingDelete}
onConfirmed={async () => {
isProcessingDelete = true
try {
await WorkspaceService.deleteWorkspace({ workspace: $workspaceStore ?? '' })
Copy link
Contributor

@claude claude bot Oct 30, 2025

Choose a reason for hiding this comment

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

Critical: Missing error handling

Same issue as the archive modal - if WorkspaceService.deleteWorkspace() throws an error, the user won't see a helpful error message and the modal will close anyway.

Recommendation:

try {
	await WorkspaceService.deleteWorkspace({ workspace: $workspaceStore ?? '' })
	sendUserToast(`Deleted workspace ${$workspaceStore}`)
	workspaceStore.set(undefined)
	usersWorkspaceStore.set(undefined)
	goto('/user/workspaces')
	deleteModalOpen = false
} catch (error) {
	sendUserToast(`Failed to delete workspace: ${error.message}`, 'error')
	// Keep modal open so user can retry
} finally {
	isProcessingDelete = false
}

Copy link
Contributor

@rubenfiszel rubenfiszel Oct 31, 2025

Choose a reason for hiding this comment

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

@tristantr claude is correct

sendUserToast(`Deleted workspace ${$workspaceStore}`)
workspaceStore.set(undefined)
usersWorkspaceStore.set(undefined)
goto('/user/workspaces')
deleteModalOpen = false
} catch (error) {
sendUserToast(`Failed to delete workspace: ${error}`, true)
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Oct 31, 2025

Choose a reason for hiding this comment

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

In the catch block for deletion, consider formatting the error message (e.g., using error.message if available) instead of interpolating the entire error object directly.

Suggested change
sendUserToast(`Failed to delete workspace: ${error}`, true)
\t sendUserToast(`Failed to delete workspace: ${error.message ?? error}`, true)

} finally {
isProcessingDelete = false
}
}}
onCanceled={() => (deleteModalOpen = false)}
>
<span>
Are you sure you want to permanently delete workspace <b class='text-emphasis font-semibold'>{$workspaceStore}</b>?
</span>
<p class="mt-2 text-emphasis text-red-600 font-semibold">
This action is irreversible and will permanently delete all data in this workspace.
</p>
</ConfirmationModal>

<style>
</style>