feat: show confirmation modal for deleting recordings and runs #319
Conversation
chore: release v0.0.5
WalkthroughThis pull request introduces significant improvements to the localization and user interface of the application. The changes focus on enhancing user experience through more robust error handling, adding confirmation dialogs for deletion actions, and expanding the localization structure. The modifications span across localization files, the recordings table, and the runs table components, with an emphasis on improving user interaction, preventing accidental deletions, and providing clearer feedback. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant DeleteConfirmation
participant Backend
User->>UI: Initiate Delete Action
UI->>DeleteConfirmation: Open Confirmation Dialog
DeleteConfirmation->>User: Prompt for Confirmation
User->>DeleteConfirmation: Confirm Deletion
DeleteConfirmation->>Backend: Execute Delete
Backend-->>UI: Deletion Completed
UI->>User: Show Success Notification
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/molecules/RunsTable.tsx (2)
132-138: Potential improvement for selective item deletion
Currently, callingsetRows([])removes all rows briefly before re-fetching them. This can cause unnecessary flicker and overhead if only one run needs to be deleted. Consider directly removing the specific run fromrowsprior to re-fetching to maintain a smoother UX.const handleDeleteConfirm = () => { - setRows([]); + setRows((prevRows) => prevRows.filter((r) => r.id !== runToDelete?.id)); notify('success', t('runstable.notifications.delete_success')); fetchRuns(); setDeleteDialogOpen(false); setRunToDelete(null); };
228-250: Hard-coded text in dialog title
Currently, "Delete Run" is hard-coded. For localization synergy, consider using something liket('runstable.delete_confirmation.title')if that exists. This will further align with the approach seen inRecordingsTable.<DialogTitle id="alert-dialog-title"> - Delete Run + {t('runstable.delete_confirmation.title', 'Delete Run')} </DialogTitle>src/components/molecules/RecordingsTable.tsx (2)
156-156: Use optional chaining here
The static analysis hint suggests using optional chaining. In line 156, you can safely check ifrecording?.recording_metaexists before accessing it, preventing potential runtime errors.- if (recording && recording.recording_meta) { + if (recording?.recording_meta) {🧰 Tools
🪛 Biome (1.9.4)
[error] 156-156: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
371-388: Delete recording flow
Deletion logic is triggered inline. While this works, you might consider a dedicated confirmation dialog—similar to theOptionsButtonapproach—for consistency. Right now, line 385 ensures errors are reported, which is good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locales/en.json(1 hunks)src/components/molecules/RecordingsTable.tsx(7 hunks)src/components/molecules/RunsTable.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/molecules/RecordingsTable.tsx
[error] 156-156: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
src/components/molecules/RunsTable.tsx (5)
12-16: Imports for Material-UI Dialog components
These imports properly introduce the required Material-UI dialog components.
88-89: Use of new state for dialog management
deleteDialogOpenandrunToDeleteare sensible additions for tracking deletion state and which run is to be removed.
127-130: Method rename and usage
handleDeleteClickcleanly shifts deletion logic into a confirmation-based flow. This readably setsrunToDeleteand opens the dialog.
140-142: Graceful dialog close
handleDeleteCancelproperly resets state when the user cancels the deletion, preserving consistency.
204-204: Delegation tohandleDeleteClick
LinkinghandleDeletetohandleDeleteClick(row)is consistent, ensuring each row triggers the Dialog.src/components/molecules/RecordingsTable.tsx (2)
178-184: Robust error handling
Catching errors onstopRecording(browserId)is beneficial. Kudos for adding user-facing notifications here, enhancing clarity when a recording fails to stop.
Line range hint
548-586: Enhanced confirmation flow inOptionsButton
This approach includes a separate confirmation dialog for deletion, ensuring user safeguards. The interplay withcheckRunsForRecordingis well-handled, but if the user has many runs, you might consider a loading state or a direct approach to gracefully handle the asynchronous checks.public/locales/en.json (4)
46-51: Addition of delete confirmation strings
Including"delete_confirmation"fosters improved user interaction by clarifying irreversible deletion steps. Good approach for multi-lingual support.
62-74: Updates to runstable notifications
Providing distinct success and fail notifications helps thoroughly convey the system state to users.
156-173: List extraction and pagination instructions
These localized strings thoroughly guide users in setting pagination and list extraction limits. This is valuable for user education.
Line range hint
568-580: Detailed error and success messages
Comprehensive error notifications, especially referencing variables (e.g.{{message}}), ensure clarity for debugging. Keep verifying placeholders to avoid mismatches.
| const handleDeleteConfirm = async () => { | ||
| try { | ||
| checkRunsForRecording(recordingName).then((result: boolean) => { | ||
| if (result) { | ||
| notify('warning', t('recordingtable.notifications.delete_warning')); | ||
| setDeleteDialogOpen(false); | ||
| } | ||
| }) | ||
|
|
||
|
|
||
|
|
||
|
|
||
| // Only proceed with deletion if there are no runs | ||
| handleDelete(); | ||
| setDeleteDialogOpen(false); | ||
| notify("success", t("recordingtable.notifications.delete_success")); | ||
| } catch (error) { | ||
| console.error('Error during deletion:', error); | ||
| notify("error", t("recordingtable.notifications.delete_error")); | ||
| setDeleteDialogOpen(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Race condition in deletion logic needs to be fixed
The verification reveals a serious race condition in the deletion logic. The code calls handleDelete() immediately after starting the checkRunsForRecording promise, without waiting for its result. This means:
- The deletion will always proceed regardless of whether there are associated runs
- The comment "Only proceed with deletion if there are no runs" is incorrect as the code doesn't enforce this condition
- The warning notification about existing runs may appear after the deletion is already completed
Here's what needs to be fixed:
- Move the
handleDelete()call inside the.then()block - Only call it when
resultisfalse(no runs exist) - Use proper
async/awaitsyntax instead of mixing with.then()
🔗 Analysis chain
Conditionally skipping actual delete
Your logic at lines 568–580 could skip the actual removal if runs are associated. As a follow-up, consider placing that skip logic inside or after the promise’s .then block, clarifying if the code still calls handleDelete() once checkRunsForRecording returns true.
const handleDeleteConfirm = async () => {
try {
checkRunsForRecording(recordingName).then((result: boolean) => {
if (result) {
notify('warning', t('recordingtable.notifications.delete_warning'));
setDeleteDialogOpen(false);
- // Potentially still calls handleDelete() below
+ return; // ensure we skip the actual delete if runs exist
}
});
handleDelete();
...Let me gather more information about the code to verify the concern.
Let me search for the actual handleDelete implementation in the RecordingsTable component since the one found is from a different component.
Let me search for the actual delete operation in the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for handleDeleteConfirm implementation and its related functions
ast-grep --pattern 'const handleDeleteConfirm = async () => {
$$$
}'
# Also search for checkRunsForRecording function to understand its implementation
ast-grep --pattern 'const checkRunsForRecording = $_'
# Search for handleDelete implementation
ast-grep --pattern 'const handleDelete = $_'
Length of output: 3090
Script:
#!/bin/bash
# Search for handleDelete in RecordingsTable.tsx
rg "const handleDelete" src/components/molecules/RecordingsTable.tsx -A 5
Length of output: 528
Script:
#!/bin/bash
# Search for delete-related operations in RecordingsTable.tsx
rg "delete" src/components/molecules/RecordingsTable.tsx -B 2 -A 5
Length of output: 3346
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes