Unify Delete UX: Add Consistent Confirmation Dialogs Across Pages and Introduce Per-Entry Delete in History#68
Open
adiudiuu wants to merge 12 commits into
Open
Unify Delete UX: Add Consistent Confirmation Dialogs Across Pages and Introduce Per-Entry Delete in History#68adiudiuu wants to merge 12 commits into
adiudiuu wants to merge 12 commits into
Conversation
- backend: invoke `sudo -n` so a host without passwordless sudo fails fast instead of feeding the SFTP byte stream to sudo as a password and hanging until the init timeout; probe sftp-server via POSIX `command -v` plus per-distro paths (Debian/Ubuntu, RHEL/Fedora, Alpine, Arch) rather than the single Debian path; record the sudo flag in tracing + telemetry. - ExplorerView: wrap the toggle in try/catch and surface failures via the error banner (was a silent no-op + unhandled rejection); add an in-flight guard plus disabled/spinner state so a double-click can't open and orphan a second server-side SFTP session. - sftp-store: preserve currentPath across the swap so the toggle reloads the same directory (as the PR describes) instead of dropping to home; re-point an outstanding clipboard to the new session id. - ExplorerToolbar: disabled/aria-busy + spinner on the shield button; coerce aria-pressed to a boolean. - UnifiedTabBar: add the missing role="tablist" container so the new role="tab" items form a valid ARIA tab set.
Add specs/55-sftp-sudo-toggle.spec.ts: - enable sudo → aria-pressed flips, label changes, listing re-renders; disable → returns symmetrically. Exercises the `sudo sftp-server` reopen and the per-distro sftp-server probe (the Alpine test image keeps it at /usr/lib/ssh/sftp-server). - toggling sudo keeps the current directory instead of bouncing to home. Add a stable data-testid="explorer-sudo-toggle" to the shield button (matching the other toolbar buttons) plus toggleSudo()/sudoToggleState() helpers in sftp-ops. Not covered: the "hidden for root" gate and the sudo-unavailable error banner — no root-login or no-sudo container exists in the e2e stack.
The e2e linuxserver images grant SUDO_ACCESS but with a PASSWORD prompt (testuser ALL=(ALL) ALL, not NOPASSWD), which exposed two gaps: - Backend: enabling sudo on a password-prompting host left the SFTP init blocked until the timeout — russh-sftp doesn't cancel the pending init on channel EOF — so the toggle hung ~30s before erroring. Add a fast `sudo -n true` preflight on a throwaway channel that returns SftpError::PermissionDenied immediately when passwordless sudo isn't configured, so the toggle surfaces a clear, fast error. - Tests: add a NOPASSWD target (tests/sudo-server, built into the e2e stack as sshd-sudo) so the toggle can actually succeed, and keep sshd-pass (password sudo) to drive the failure path. Rewrite 55-sftp-sudo-toggle into three cases: enable/disable, directory preservation, and the error-banner path. Tag the explorer error banner with a data-testid.
The preflight loop broke on ChannelMsg::Eof, but sshd commonly sends Eof BEFORE the exit-status request — so sudo_exit stayed None and EVERY host, including one with passwordless sudo, was rejected as PermissionDenied (the e2e sshd-sudo case failed for exactly this reason). Break only on Close so the exit status is actually captured.
The PR bumped the SFTP init timeout to 30s via new_opts(.., Some(30)). russh's request_subsystem returns BEFORE the server's accept/reject reply, so on an SCP-only host (SFTP subsystem stripped) the SFTP init is what fails — and the 30s timeout delayed the frontend's SFTP->SCP fallback past the e2e's 30s waitForExplorer, failing all 6 SCP-fallback specs (52-scp-flow x2 images). Revert to SftpSession::new (10s default, matching main) so the fallback fires promptly. The sudo path is unaffected: init is fast once the `sudo -n true` preflight confirms passwordless sudo.
# Conflicts: # src/components/history/HistoryPage.tsx
…try Delete in History
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds a reusable danger confirmation dialog and wires it into multiple “Delete” actions across the UI, including a new backend command to delete individual connection history entries.
Changes:
- Introduced a shared
ConfirmDangerDialogcomponent for destructive action confirmation. - Updated various cards/pages to open the confirmation dialog before executing delete actions.
- Added a new Tauri backend DB command (
delete_connection_history_entry) and frontend integration inHistoryPage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/shared/ConfirmDangerDialog.tsx | Adds the shared confirmation modal used for destructive actions. |
| src/components/snippets/SnippetCard.tsx | Routes snippet deletion through the confirmation dialog. |
| src/components/snippets/SnippetFolderCard.tsx | Routes folder deletion through the confirmation dialog. |
| src/components/dashboard/HostCard.tsx | Routes host deletion through the confirmation dialog. |
| src/components/dashboard/S3Card.tsx | Routes S3 connection deletion through the confirmation dialog. |
| src/components/s3/S3Page.tsx | Adds confirmation flow for deleting saved S3 connections from the page context menu. |
| src/components/port-forwarding/PortForwardingPage.tsx | Adds confirmation flow for deleting port-forwarding rules. |
| src/components/history/HistoryPage.tsx | Adds history-row delete UI + calls the new backend delete command. |
| src-tauri/src/lib.rs | Registers the new Tauri command. |
| src-tauri/src/db/mod.rs | Implements DB deletion for a single history entry by id. |
| src-tauri/src/db/commands.rs | Exposes the delete command to the frontend via Tauri. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+32
| <div className="fixed inset-0 z-50 flex items-center justify-center" aria-modal="true" role="dialog" aria-labelledby="confirm-danger-title"> | ||
| <div | ||
| className="absolute inset-0 bg-bg-base/70 backdrop-blur-sm" | ||
| onClick={onCancel} | ||
| aria-hidden="true" | ||
| /> |
Comment on lines
+47
to
+62
| <button | ||
| onClick={onCancel} | ||
| className={[ | ||
| "px-4 py-1.5 rounded-lg text-[length:var(--text-sm)] font-medium", | ||
| "text-text-secondary bg-bg-overlay border border-border", | ||
| "hover:text-text-primary hover:border-border-focus hover:bg-bg-subtle", | ||
| "transition-all duration-[var(--duration-fast)]", | ||
| "focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring", | ||
| ].join(" ")} | ||
| > | ||
| {cancelLabel} | ||
| </button> | ||
| <button | ||
| onClick={onConfirm} | ||
| disabled={busy} | ||
| className={[ |
| if (!open) return null; | ||
|
|
||
| return ( | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center" aria-modal="true" role="dialog" aria-labelledby="confirm-danger-title"> |
Comment on lines
+39
to
+42
| <h2 id="confirm-danger-title" className="text-[length:var(--text-sm)] font-semibold text-text-primary"> | ||
| {title} | ||
| </h2> | ||
| <p className="text-[length:var(--text-xs)] text-text-muted mt-1">{message}</p> |
Comment on lines
79
to
84
| { | ||
| label: "Delete", | ||
| icon: Trash2, | ||
| danger: true, | ||
| onClick: () => onDelete(snippet.id), | ||
| onClick: () => setConfirmDelete(true), | ||
| }, |
Comment on lines
+185
to
+191
| { | ||
| label: "Delete", | ||
| icon: Trash2, | ||
| danger: true, | ||
| disabled: mutating, | ||
| onClick: () => setConfirmDeleteId(entry.id), | ||
| }, |
| <AlertTriangle size={18} strokeWidth={1.8} className="text-status-error" aria-hidden="true" /> | ||
| </div> | ||
| <div> | ||
| <h2 id="confirm-danger-title" className="text-[length:var(--text-sm)] font-semibold text-text-primary"> |
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
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.
Background
Several pages in the project support delete actions, but confirmation behavior is inconsistent:
This change standardizes delete confirmations with one in-app confirmation dialog pattern to reduce accidental deletions and improve UX consistency.
What Changed
1) Added a shared danger confirmation dialog
src/components/shared/ConfirmDangerDialog.tsxCancel/Confirm)2) History page
Deletefrom context menu)3) Unified delete confirmations on Dashboard
src/components/dashboard/HostCard.tsxsrc/components/dashboard/S3Card.tsx4) Port Forwarding page
src/components/port-forwarding/PortForwardingPage.tsx5) S3 page
src/components/s3/S3Page.tsx6) Snippets page
src/components/snippets/SnippetCard.tsxsrc/components/snippets/SnippetFolderCard.tsx7) Backend delete API status
src-tauri/src/db/mod.rssrc-tauri/src/db/commands.rssrc-tauri/src/lib.rsScope
Risk and Compatibility
Validation Checklist
Please verify the following flows:
Delete-> dialog ->Confirmdeletes only the selected entry.Delete-> dialog ->Confirmdeletes the host.Delete-> dialog ->Confirmdeletes the connection.Delete-> dialog ->Confirmdeletes the rule.Delete-> dialog ->Confirmdeletes the target.Cancelor clicking the backdrop does not perform deletion.Changed Files
src/components/shared/ConfirmDangerDialog.tsxsrc/components/history/HistoryPage.tsxsrc/components/dashboard/HostCard.tsxsrc/components/dashboard/S3Card.tsxsrc/components/port-forwarding/PortForwardingPage.tsxsrc/components/s3/S3Page.tsxsrc/components/snippets/SnippetCard.tsxsrc/components/snippets/SnippetFolderCard.tsxsrc-tauri/src/db/mod.rssrc-tauri/src/db/commands.rssrc-tauri/src/lib.rs