notes: replace native confirm() with styled AlertDialog (task #423)
Original task: deleting a note (and a label from the manage-labels menu)
showed the browser's native confirm() with the raw Repl hostname,
clashing with the rest of the app. Switch both call sites to the
project's existing AlertDialog component, matching the pattern used in
FoldersRail and SendNoteDialog.
Changes
- artifacts/tx-os/src/pages/notes.tsx
- NoteCard: add `confirmDeleteOpen` state; render AlertDialog as a
Fragment sibling of the clickable card (not a child) so React's
portal event bubbling cannot trigger the card's onEdit handler when
the user interacts with the dialog. Also stopPropagation on the
trash button itself.
- LabelsDialog: add `pendingDeleteLabel` state and a sibling
AlertDialog inside the Dialog root. Both dialogs use existing
locale keys (notes.deleteConfirm, notes.deleteLabelConfirm,
notes.delete, common.cancel) — no new strings needed.
- Add data-testids: note-card-delete-{id}, note-delete-dialog-{id},
note-delete-confirm-{id}, label-delete-{id},
label-delete-dialog-{id}, label-delete-confirm-{id},
notes-manage-labels-open.
Tests
- artifacts/tx-os/tests/notes-delete-confirm.spec.mjs (new)
- Note delete: cancel keeps the card, confirm fires DELETE and
removes the card. Deliberately does NOT register a `dialog` event
handler — a native confirm would hang the test.
- Label delete: open manage-labels, create a label via the UI, click
trash, confirm, assert DELETE /api/note-labels/{id}.
Verification
- pnpm --filter @workspace/tx-os exec tsc --noEmit: clean.
- pnpm --filter @workspace/tx-os test:e2e -- notes-delete-confirm:
2 passed.
- Architect code review surfaced a real bubble-through risk on the
card's onClick={onEdit}; fixed by lifting the AlertDialog to a
Fragment sibling.
Out of scope (not changed): native confirm()/alert() outside the Notes
page. (executive-meetings.tsx already uses an in-app `confirm({...})`
modal, not the native browser dialog.)
This commit is contained in:
@@ -258,6 +258,7 @@ export default function NotesPage() {
|
||||
size="sm"
|
||||
variant="ghost"
|
||||
onClick={() => setLabelsDialogOpen(true)}
|
||||
data-testid="notes-manage-labels-open"
|
||||
>
|
||||
<Tag size={16} className="me-1" />
|
||||
{t("notes.labels", "Labels")}
|
||||
@@ -544,6 +545,7 @@ function NoteCard({
|
||||
const update = useUpdateNote();
|
||||
const del = useDeleteNote();
|
||||
const noteLabels = labels.filter((l) => note.labelIds.includes(l.id));
|
||||
const [confirmDeleteOpen, setConfirmDeleteOpen] = useState(false);
|
||||
|
||||
// Touch fallback for mobile / iPad: a long-press starts a drag, touchmove
|
||||
// hit-tests folder drop targets, touchend resolves the drop.
|
||||
@@ -567,6 +569,7 @@ function NoteCard({
|
||||
};
|
||||
|
||||
return (
|
||||
<>
|
||||
<div
|
||||
data-testid={`note-card-${note.id}`}
|
||||
data-folder-id={note.folderId ?? ""}
|
||||
@@ -724,18 +727,48 @@ function NoteCard({
|
||||
{note.isArchived ? <ArchiveRestore size={15} /> : <Archive size={15} />}
|
||||
</button>
|
||||
<button
|
||||
onClick={() => {
|
||||
if (confirm(t("notes.deleteConfirm", "Delete this note?"))) {
|
||||
del.mutate(note.id);
|
||||
}
|
||||
onClick={(e) => {
|
||||
e.stopPropagation();
|
||||
setConfirmDeleteOpen(true);
|
||||
}}
|
||||
className="p-1 rounded-md hover:bg-black/5 text-muted-foreground hover:text-destructive"
|
||||
aria-label={t("notes.delete", "Delete")}
|
||||
data-testid={`note-card-delete-${note.id}`}
|
||||
>
|
||||
<Trash2 size={15} />
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
{/* AlertDialog rendered as a sibling (not a child of the clickable card)
|
||||
so React's portal event bubbling can't trigger the card's onEdit
|
||||
click handler when the user interacts with the confirmation. */}
|
||||
<AlertDialog
|
||||
open={confirmDeleteOpen}
|
||||
onOpenChange={setConfirmDeleteOpen}
|
||||
>
|
||||
<AlertDialogContent data-testid={`note-delete-dialog-${note.id}`}>
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>
|
||||
{t("notes.deleteConfirm", "Delete this note?")}
|
||||
</AlertDialogTitle>
|
||||
</AlertDialogHeader>
|
||||
<AlertDialogFooter>
|
||||
<AlertDialogCancel>
|
||||
{t("common.cancel", "Cancel")}
|
||||
</AlertDialogCancel>
|
||||
<AlertDialogAction
|
||||
onClick={() => {
|
||||
del.mutate(note.id);
|
||||
setConfirmDeleteOpen(false);
|
||||
}}
|
||||
data-testid={`note-delete-confirm-${note.id}`}
|
||||
>
|
||||
{t("notes.delete", "Delete")}
|
||||
</AlertDialogAction>
|
||||
</AlertDialogFooter>
|
||||
</AlertDialogContent>
|
||||
</AlertDialog>
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -2005,6 +2038,9 @@ function LabelsDialog({
|
||||
const [name, setName] = useState("");
|
||||
const [editingId, setEditingId] = useState<number | null>(null);
|
||||
const [editingName, setEditingName] = useState("");
|
||||
const [pendingDeleteLabel, setPendingDeleteLabel] = useState<NoteLabel | null>(
|
||||
null,
|
||||
);
|
||||
const create = useCreateLabel();
|
||||
const update = useUpdateLabel();
|
||||
const del = useDeleteLabel();
|
||||
@@ -2095,12 +2131,9 @@ function LabelsDialog({
|
||||
{l.name}
|
||||
</span>
|
||||
<button
|
||||
onClick={() => {
|
||||
if (confirm(t("notes.deleteLabelConfirm", "Delete this label?"))) {
|
||||
del.mutate(l.id);
|
||||
}
|
||||
}}
|
||||
onClick={() => setPendingDeleteLabel(l)}
|
||||
className="p-1 text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100"
|
||||
data-testid={`label-delete-${l.id}`}
|
||||
>
|
||||
<Trash2 size={14} />
|
||||
</button>
|
||||
@@ -2110,6 +2143,46 @@ function LabelsDialog({
|
||||
))}
|
||||
</div>
|
||||
</DialogContent>
|
||||
<AlertDialog
|
||||
open={pendingDeleteLabel !== null}
|
||||
onOpenChange={(o) => {
|
||||
if (!o) setPendingDeleteLabel(null);
|
||||
}}
|
||||
>
|
||||
<AlertDialogContent
|
||||
data-testid={
|
||||
pendingDeleteLabel
|
||||
? `label-delete-dialog-${pendingDeleteLabel.id}`
|
||||
: "label-delete-dialog"
|
||||
}
|
||||
>
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>
|
||||
{t("notes.deleteLabelConfirm", "Delete this label?")}
|
||||
</AlertDialogTitle>
|
||||
</AlertDialogHeader>
|
||||
<AlertDialogFooter>
|
||||
<AlertDialogCancel>
|
||||
{t("common.cancel", "Cancel")}
|
||||
</AlertDialogCancel>
|
||||
<AlertDialogAction
|
||||
onClick={() => {
|
||||
if (pendingDeleteLabel) {
|
||||
del.mutate(pendingDeleteLabel.id);
|
||||
setPendingDeleteLabel(null);
|
||||
}
|
||||
}}
|
||||
data-testid={
|
||||
pendingDeleteLabel
|
||||
? `label-delete-confirm-${pendingDeleteLabel.id}`
|
||||
: "label-delete-confirm"
|
||||
}
|
||||
>
|
||||
{t("notes.delete", "Delete")}
|
||||
</AlertDialogAction>
|
||||
</AlertDialogFooter>
|
||||
</AlertDialogContent>
|
||||
</AlertDialog>
|
||||
</Dialog>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,176 @@
|
||||
// E2E regression for the styled in-app delete confirmation on notes.
|
||||
// Previously the trash icon called the browser's native confirm() which
|
||||
// shows the raw Repl hostname and clashes with the rest of the app's
|
||||
// design. This test guards the styled AlertDialog: cancelling keeps the
|
||||
// note, confirming deletes it. We deliberately do NOT register a
|
||||
// `dialog` event handler — if a native confirm fires the test will hang
|
||||
// (and fail), proving the native dialog is gone.
|
||||
import { test, expect } from "@playwright/test";
|
||||
import pg from "pg";
|
||||
|
||||
const DATABASE_URL = process.env.DATABASE_URL;
|
||||
if (!DATABASE_URL) {
|
||||
throw new Error("DATABASE_URL must be set to run this UI test");
|
||||
}
|
||||
|
||||
const TEST_PASSWORD = "TestPass123!";
|
||||
const TEST_PASSWORD_HASH =
|
||||
"$2b$10$Bs636ukPMyz01nKrsi.5m.JlDXSN22AVCvn8cgPWWDbo5yJRQX2vu";
|
||||
|
||||
const pool = new pg.Pool({ connectionString: DATABASE_URL });
|
||||
const createdUserIds = [];
|
||||
|
||||
function uniq() {
|
||||
return `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`;
|
||||
}
|
||||
|
||||
async function createUser(prefix, displayEn) {
|
||||
const username = `${prefix}_${uniq()}`;
|
||||
const { rows } = await pool.query(
|
||||
`INSERT INTO users (username, email, password_hash, display_name_en, preferred_language, is_active)
|
||||
VALUES ($1, $2, $3, $4, 'en', true) RETURNING id`,
|
||||
[username, `${username}@example.com`, TEST_PASSWORD_HASH, displayEn],
|
||||
);
|
||||
const id = rows[0].id;
|
||||
createdUserIds.push(id);
|
||||
await pool.query(
|
||||
`INSERT INTO user_roles (user_id, role_id)
|
||||
SELECT $1, id FROM roles WHERE name = 'user'`,
|
||||
[id],
|
||||
);
|
||||
return { id, username };
|
||||
}
|
||||
|
||||
test.afterAll(async () => {
|
||||
if (createdUserIds.length > 0) {
|
||||
await pool.query(`DELETE FROM notes WHERE user_id = ANY($1::int[])`, [
|
||||
createdUserIds,
|
||||
]);
|
||||
await pool.query(`DELETE FROM user_roles WHERE user_id = ANY($1::int[])`, [
|
||||
createdUserIds,
|
||||
]);
|
||||
await pool.query(`DELETE FROM users WHERE id = ANY($1::int[])`, [
|
||||
createdUserIds,
|
||||
]);
|
||||
}
|
||||
await pool.end();
|
||||
});
|
||||
|
||||
async function loginViaUi(page, username) {
|
||||
await page.goto("/login");
|
||||
await page.locator("#username").fill(username);
|
||||
await page.locator("#password").fill(TEST_PASSWORD);
|
||||
await Promise.all([
|
||||
page.waitForURL((url) => !url.pathname.endsWith("/login"), {
|
||||
timeout: 15_000,
|
||||
}),
|
||||
page.locator('form button[type="submit"]').click(),
|
||||
]);
|
||||
}
|
||||
|
||||
async function createNoteViaComposer(page, title) {
|
||||
await page.getByTestId("notes-composer").click();
|
||||
await page.getByTestId("notes-composer-title").fill(title);
|
||||
const savePromise = page.waitForResponse(
|
||||
(r) =>
|
||||
new URL(r.url()).pathname === "/api/notes" &&
|
||||
r.request().method() === "POST" &&
|
||||
r.status() >= 200 &&
|
||||
r.status() < 300,
|
||||
);
|
||||
await page.mouse.click(10, 10);
|
||||
const resp = await savePromise;
|
||||
return resp.json();
|
||||
}
|
||||
|
||||
test("delete note uses styled confirmation dialog (cancel keeps it, confirm deletes)", async ({
|
||||
page,
|
||||
}) => {
|
||||
const user = await createUser("notes_delete_confirm", "Delete Tester");
|
||||
await loginViaUi(page, user.username);
|
||||
|
||||
await page.goto("/notes");
|
||||
await expect(page.getByTestId("notes-page")).toBeVisible();
|
||||
|
||||
const title = `delete-test ${uniq()}`;
|
||||
const note = await createNoteViaComposer(page, title);
|
||||
|
||||
const card = page.getByTestId(`note-card-${note.id}`);
|
||||
await expect(card).toBeVisible();
|
||||
|
||||
// Open the styled confirmation. Hover first so the action row appears.
|
||||
await card.hover();
|
||||
await page.getByTestId(`note-card-delete-${note.id}`).click();
|
||||
|
||||
// Styled dialog appears (no native confirm).
|
||||
const dialog = page.getByTestId(`note-delete-dialog-${note.id}`);
|
||||
await expect(dialog).toBeVisible();
|
||||
|
||||
// Cancel — note must still be there.
|
||||
await dialog.getByRole("button", { name: "Cancel" }).click();
|
||||
await expect(dialog).toBeHidden();
|
||||
await expect(card).toBeVisible();
|
||||
|
||||
// Re-open and confirm.
|
||||
await card.hover();
|
||||
await page.getByTestId(`note-card-delete-${note.id}`).click();
|
||||
await expect(page.getByTestId(`note-delete-dialog-${note.id}`)).toBeVisible();
|
||||
|
||||
const deletePromise = page.waitForResponse(
|
||||
(r) =>
|
||||
new URL(r.url()).pathname === `/api/notes/${note.id}` &&
|
||||
r.request().method() === "DELETE" &&
|
||||
r.status() >= 200 &&
|
||||
r.status() < 300,
|
||||
);
|
||||
await page.getByTestId(`note-delete-confirm-${note.id}`).click();
|
||||
await deletePromise;
|
||||
|
||||
await expect(card).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("delete label uses styled confirmation dialog (cancel keeps it, confirm deletes)", async ({
|
||||
page,
|
||||
}) => {
|
||||
const user = await createUser("notes_label_delete", "Label Tester");
|
||||
await loginViaUi(page, user.username);
|
||||
|
||||
await page.goto("/notes");
|
||||
await expect(page.getByTestId("notes-page")).toBeVisible();
|
||||
|
||||
// Open the manage-labels dialog and create a label through the UI so
|
||||
// React Query has the row in its cache by the time we go to delete it.
|
||||
await page.getByTestId("notes-manage-labels-open").click();
|
||||
|
||||
const labelName = `lbl-${uniq()}`;
|
||||
const createPromise = page.waitForResponse(
|
||||
(r) =>
|
||||
new URL(r.url()).pathname === "/api/note-labels" &&
|
||||
r.request().method() === "POST" &&
|
||||
r.status() >= 200 &&
|
||||
r.status() < 300,
|
||||
);
|
||||
await page.getByPlaceholder(/new label|تصنيف جديد/i).fill(labelName);
|
||||
await page.getByPlaceholder(/new label|تصنيف جديد/i).press("Enter");
|
||||
const label = await (await createPromise).json();
|
||||
|
||||
// The trash button uses opacity-0 + group-hover; force the click since
|
||||
// hovering a per-row group on portaled dialogs is flaky in headless.
|
||||
const labelDeleteBtn = page.getByTestId(`label-delete-${label.id}`);
|
||||
await labelDeleteBtn.click({ force: true });
|
||||
|
||||
const dialog = page.getByTestId(`label-delete-dialog-${label.id}`);
|
||||
await expect(dialog).toBeVisible();
|
||||
|
||||
const deletePromise = page.waitForResponse(
|
||||
(r) =>
|
||||
new URL(r.url()).pathname === `/api/note-labels/${label.id}` &&
|
||||
r.request().method() === "DELETE" &&
|
||||
r.status() >= 200 &&
|
||||
r.status() < 300,
|
||||
);
|
||||
await page.getByTestId(`label-delete-confirm-${label.id}`).click();
|
||||
await deletePromise;
|
||||
|
||||
await expect(labelDeleteBtn).toHaveCount(0);
|
||||
});
|
||||
Reference in New Issue
Block a user