feat: Add modal for critical actions

[tristantr]

Add confirmation modals for 2 critical actions : Archiving and Deleting a workspace

[claude]

Claude finished @tristantr's task —— View job

---

Code Review Complete

Tasks

• Read the modified workspace settings file
• Analyze the git diff to understand changes
• Review code quality and Svelte 5 best practices
• Check for potential bugs and security issues
• Evaluate performance considerations
• Provide detailed feedback
• Add reproduction instructions

---

Review Summary

This PR successfully adds confirmation modals for two critical workspace actions: archiving and deleting. The implementation follows good safety practices by requiring explicit user confirmation before executing destructive operations.

Positive Aspects

✅ Good security practice: Adding confirmation modals for critical actions prevents accidental data loss
✅ Consistent UX: Uses the existing ConfirmationModal component for a consistent user experience
✅ Proper error handling: Uses try-finally blocks to ensure cleanup even if operations fail
✅ Loading states: Implements loading state with isProcessingWorkspaceDeletion to prevent double-clicks
✅ Clear messaging: The delete modal includes a strong warning about irreversibility
✅ Svelte 5 compliance: Correctly uses $state rune for reactive state management

Issues Found

1. Shared Loading State Bug (Medium Priority)

Location: Lines 64-66, 1052-1063, 1078-1090

Issue: Both modals share the same isProcessingWorkspaceDeletion state variable. This creates a potential race condition and UI bug:

• If a user somehow triggers both modals (unlikely but possible), the loading state would be shared
• More importantly, if one modal's operation fails during the try block before reaching the finally, it could leave the other modal in a loading state

Recommendation:

let isProcessingArchive = $state(false)
let isProcessingDelete = $state(false)

Then use isProcessingArchive for the archive modal and isProcessingDelete for the delete modal.

2. Missing Error Handling (Medium Priority)

Location: Lines 1053-1064, 1079-1090

Issue: Both onConfirmed handlers don't catch errors. If WorkspaceService.archiveWorkspace() or WorkspaceService.deleteWorkspace() throws an error:

• The user won't see a helpful error message
• The modal will close (due to finally block) even though the operation failed
• The loading state will be cleared, making it appear the operation succeeded

Recommendation: Add error handling:

onConfirmed={async () => {
	isProcessingArchive = true
	try {
		await WorkspaceService.archiveWorkspace({ workspace: $workspaceStore ?? '' })
		sendUserToast(`Archived workspace ${$workspaceStore}`)
		workspaceStore.set(undefined)
		usersWorkspaceStore.set(undefined)
		goto('/user/workspaces')
	} catch (error) {
		sendUserToast(`Failed to archive workspace: ${error.message}`, 'error')
		// Don't close modal or navigate on error
	} finally {
		isProcessingArchive = false
		archiveModalOpen = false // Only close on success or error, user can try again
	}
}}

3. Modal Closure Logic Issue (Low Priority)

Location: Lines 1063, 1089

Issue: The modals are closed in the finally block, which means they close even if the operation fails. This is inconsistent with the error handling recommendation above. If we add proper error handling, we should only close the modal on success, allowing users to see the error and try again.

Current behavior: Modal closes whether operation succeeds or fails
Recommended behavior: Modal stays open on error, allowing user to retry or cancel

4. Comment Accuracy (Minor)

Location: Line 63

Issue: The comment says "Modal state for workspace deletion" but it actually covers both archive and delete operations.

Recommendation:

// Modal state for workspace archive/delete operations

Performance Considerations

✅ Good: Loading state prevents multiple simultaneous requests
✅ Good: Modal components only render when open={true}
✅ Good: Reuses existing ConfirmationModal component (no code duplication)

No performance concerns identified.

Security Considerations

✅ Good: Backend validation still applies (these are just UI confirmations)
✅ Good: Superadmin check for delete operation is handled correctly
⚠️ Note: The delete operation only checks $superadmin on the frontend. Ensure backend also validates superadmin status (this is likely already in place, but worth confirming).

Svelte 5 Best Practices

✅ Correctly uses $state rune for reactive state
✅ Properly binds reactive variables to component props
✅ Uses async arrow functions appropriately
✅ No deprecated Svelte 4 patterns detected

Additional Observations

1. Consistent patterns: The implementation follows existing patterns in the codebase (e.g., similar to how other confirmation modals are used)
2. Accessibility: The ConfirmationModal component supports keyboard shortcuts (Enter/Escape), which is good for accessibility
3. Type safety: Modal state variables have proper type annotations via $state(false)

---

Recommendations Summary

Must Fix:

• Separate the loading states for archive and delete modals
• Add proper error handling to prevent silent failures

Should Fix:

• Keep modal open on error to allow retry
• Update comment to reflect both operations

Nice to Have:

• Consider adding a text input confirmation (e.g., "type workspace name to confirm") for the delete operation, given its irreversibility

---

Testing Instructions

To verify these changes work correctly, navigate to the Windmill workspace settings page by clicking on your workspace name and selecting "Workspace Settings" from the dropdown. Then go to the "General" tab where you'll find the workspace management section at the bottom of the page. Click the "Archive workspace" button and verify that a confirmation modal appears asking you to confirm the archival, with clear messaging that this action can be reversed by a superadmin. Cancel the modal and verify it closes properly. For superadmin users, also test the "Delete workspace (superadmin)" button which should show a more severe warning modal indicating the action is permanent and irreversible with emphasized red text. Verify both modals show loading states when confirmed and properly navigate to the workspaces list upon successful completion. Also test that the buttons are appropriately disabled for the special "admins" and "starter" workspaces.

---

[ellipsis-dev]

Consider using separate processing states (or a more generic name) for archiving and deletion. The current isProcessingWorkspaceDeletion is used in both modals, which may be confusing.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	484350c
Status:	✅  Deploy successful!
Preview URL:	https://5e7cbb2d.windmill.pages.dev
Branch Preview URL:	https://tl-modals-for-critical-actio.windmill.pages.dev

View logs

[claude]

Issue: Shared loading state between modals

Both the archive and delete modals share isProcessingWorkspaceDeletion. This could cause UI bugs if both operations could somehow be triggered, or if one fails partway through.

Recommendation: Use separate loading states:

let isProcessingArchive = $state(false)
let isProcessingDelete = $state(false)

[claude]

Minor: Inaccurate comment

The comment says "workspace deletion" but these state variables handle both archive and delete operations.

Suggestion:

// Modal state for workspace archive/delete operations

[claude]

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.

[rubenfiszel]

@tristantr claude is correct

[claude]

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
}

[rubenfiszel]

@tristantr claude is correct

[ellipsis-dev]

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)

[ellipsis-dev]

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)