From 359bac149b66ce613e941f025f5eb4c7f5ee131f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 11 Aug 2026 20:25:18 +0200 Subject: [PATCH] refactor: address /code-review on R11 feedback symmetry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - admin._decide: add an idempotency guard (409 unless status was 'pending') so a repeat/stale approve OR deny can't re-run its side effects — now that deny emails, this stops duplicate denial mails and a rejection mail to an already-approved user. - SavedViewsWidget: extract runViewMutation(fn, failMsg) (returns success) — dedups the five try/catch+refresh blocks AND lets create/rename close their input only on success, so a failed create keeps the user's typed name. - worker._notify_download_done: take user_id/title captured BEFORE the caller's commit (no post-commit refresh SELECT); add a so the inbox labels an edit job 'Edit ready' instead of 'Download ready' (new inbox.editDone i18n, HU+EN). Gate green (ruff/tsc/eslint-0/prettier/knip/vitest 79/pytest 193); smoke-tested the saved-views + notifications surfaces render clean on a fresh dev server. --- backend/app/routes/admin.py | 6 ++ backend/app/worker.py | 33 +++++---- .../src/components/NotificationsPanel.tsx | 7 +- frontend/src/components/SavedViewsWidget.tsx | 69 ++++++++++--------- frontend/src/i18n/locales/en/inbox.json | 3 + frontend/src/i18n/locales/hu/inbox.json | 3 + 6 files changed, 72 insertions(+), 49 deletions(-) diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 12425b87..3a1f5ad9 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -74,6 +74,12 @@ def _decide(db: Session, invite_id: int, admin: User, approved: bool) -> Invite: inv = db.get(Invite, invite_id) if inv is None: raise HTTPException(status_code=404, detail="No such invite") + # Idempotency guard: only a still-pending invite can be decided. Without this a repeat/stale + # approve or deny re-runs its side effects — re-activating a user, or (now that deny emails) + # sending a duplicate decision mail, even flipping an already-approved invite to "denied" and + # mailing a rejection to an already-active account. + if inv.status != "pending": + raise HTTPException(status_code=409, detail="Invite already decided") inv.status = "approved" if approved else "denied" inv.decided_at = datetime.now(timezone.utc) inv.decided_by = admin.email diff --git a/backend/app/worker.py b/backend/app/worker.py index 9ae998b8..846f7d7b 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -40,24 +40,28 @@ from app.titles import normalize_title log = logging.getLogger("siftlode.worker") -def _notify_download_done(db, job: DownloadJob | None, title: str | None) -> None: - """Post a best-effort 'download ready' notice to the owner's inbox — downloads used to slide to - Done silently while scheduler jobs and playlist-push already notified (R11 feedback symmetry). - A notification failure must never mask the completed download, so it is isolated + rolled back - (mirrors scheduler._notify_done). English title is a fallback; the inbox localizes from `data`.""" - if not job: +def _notify_download_done( + db, user_id: int | None, title: str | None, job_id, *, kind: str +) -> None: + """Post a best-effort 'ready' notice to the owner's inbox — downloads/edits used to finish + silently while scheduler jobs and playlist-push already notified (R11 feedback symmetry). A + notification failure must never mask the completed job, so it is isolated + rolled back (mirrors + scheduler._notify_done). `kind` ("download"/"edit") lets the inbox pick accurate copy; the + English title here is only a fallback for an unknown type. `user_id`/`title` are captured by the + caller BEFORE its commit, so reading them here needs no post-commit refresh of an expired row.""" + if user_id is None: return try: create_notification( db, - user_id=job.user_id, + user_id=user_id, type="download_done", - title=f"Download ready: {title}" if title else "Download ready", - data={"title": title}, + title=f"Ready to watch: {title}" if title else "Ready to watch", + data={"title": title, "kind": kind}, ) except Exception: db.rollback() - log.exception("failed to write download-done notification for job %s", job.id) + log.exception("failed to write download-done notification for job %s", job_id) _stop = threading.Event() @@ -657,8 +661,10 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe if not job.display_name: # display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early). job.display_name = meta.title[:255] if meta.title else None + # Capture the owner before commit expires the row (avoids a refresh SELECT just to notify). + owner_id = job.user_id if job else None db.commit() - _notify_download_done(db, job, meta.title) + _notify_download_done(db, owner_id, meta.title, job_id, kind="download") log.info("job %s done: %s", job_id, rel) except (_Aborted, editmod.EditAborted): @@ -790,8 +796,11 @@ def _process_edit(job_id: int, asset_id: int) -> None: job.status = "done" job.progress = 100 job.phase = None + # Capture owner + title before commit expires the rows (avoids a refresh SELECT). + owner_id = job.user_id if job else None + dl_title = asset.title if asset else None db.commit() - _notify_download_done(db, job, asset.title if asset else None) + _notify_download_done(db, owner_id, dl_title, job_id, kind="edit") log.info("edit job %s done: %s", job_id, rel) except editmod.EditAborted: diff --git a/frontend/src/components/NotificationsPanel.tsx b/frontend/src/components/NotificationsPanel.tsx index e0b15c34..c0b8eecf 100644 --- a/frontend/src/components/NotificationsPanel.tsx +++ b/frontend/src/components/NotificationsPanel.tsx @@ -259,9 +259,10 @@ function NotificationRow({ }); body = d.summary || n.body; } else if (isDownloadDone) { - // "Download ready", with the video's title as the body (the producer ships it in data.title). - const d = n.data as { title?: string | null }; - title = t("inbox.downloadDone.title"); + // "Download ready" / "Edit ready" per the producer's kind, with the video's title as the body + // (shipped in data.title). Same notification type; the copy differs so an edit isn't mislabelled. + const d = n.data as { title?: string | null; kind?: string }; + title = t(d.kind === "edit" ? "inbox.editDone.title" : "inbox.downloadDone.title"); body = d.title || n.body; } return ( diff --git a/frontend/src/components/SavedViewsWidget.tsx b/frontend/src/components/SavedViewsWidget.tsx index b6d4cccc..44d6b6dd 100644 --- a/frontend/src/components/SavedViewsWidget.tsx +++ b/frontend/src/components/SavedViewsWidget.tsx @@ -72,6 +72,22 @@ export default function SavedViewsWidget({ }, [viewsQuery.data]); const refresh = () => qc.invalidateQueries({ queryKey: qk.savedViews() }); + + // Run a saved-view mutation with uniform feedback: on failure surface the server's reason instead + // of a silent swallow, and always refetch afterwards (which also reverts an optimistic change to + // the server's truth). Returns whether it succeeded, so callers can gate success-only side effects + // (e.g. closing an editor) and keep the input open — with the user's text — on failure. + async function runViewMutation(fn: () => Promise, failMsg: string): Promise { + try { + await fn(); + return true; + } catch (e) { + notifyActionError(e, failMsg); + return false; + } finally { + refresh(); + } + } const currentKey = keyOf(filters); async function save() { @@ -79,35 +95,30 @@ export default function SavedViewsWidget({ if (!name) return; // Strip the transient shuffle seed so a saved "shuffle" view re-rolls fresh on apply. const { seed: _seed, ...clean } = filters; - setNaming(false); - setDraftName(""); - try { - await api.createView(name, clean); - } catch (e) { - notifyActionError(e, t("views.actionFailed")); + // Close the naming input only on success, so a failed create keeps the user's typed name. + if (await runViewMutation(() => api.createView(name, clean), t("views.actionFailed"))) { + setNaming(false); + setDraftName(""); } - refresh(); } async function rename(id: number, name: string) { const next = name.trim(); - setRenameId(null); - if (!next) return; - try { - await api.updateView(id, { name: next }); - } catch (e) { - notifyActionError(e, t("views.actionFailed")); + if (!next) { + setRenameId(null); + return; + } + // Close the rename editor only on success, so a failed rename stays open for a retry. + if (await runViewMutation(() => api.updateView(id, { name: next }), t("views.actionFailed"))) { + setRenameId(null); } - refresh(); } async function toggleDefault(v: SavedView) { - try { - await api.updateView(v.id, { is_default: !v.is_default }); - } catch (e) { - notifyActionError(e, t("views.actionFailed")); - } - refresh(); + await runViewMutation( + () => api.updateView(v.id, { is_default: !v.is_default }), + t("views.actionFailed"), + ); } async function remove(v: SavedView) { @@ -117,12 +128,7 @@ export default function SavedViewsWidget({ danger: true, }); if (!ok) return; - try { - await api.deleteView(v.id); - } catch (e) { - notifyActionError(e, t("views.actionFailed")); - } - refresh(); + await runViewMutation(() => api.deleteView(v.id), t("views.actionFailed")); } async function share(v: SavedView) { @@ -146,14 +152,9 @@ export default function SavedViewsWidget({ qk.savedViews(), next.map((id) => views.find((v) => v.id === id)!), ); - // On failure the refetch below reverts the optimistic order to the server's truth; without the - // toast the drop would silently snap back with no explanation. - try { - await api.reorderViews(next); - } catch (e) { - notifyActionError(e, t("views.reorderFailed")); - } - refresh(); + // On failure runViewMutation's refetch reverts the optimistic order to the server's truth; + // without the toast the drop would silently snap back with no explanation. + await runViewMutation(() => api.reorderViews(next), t("views.reorderFailed")); } return ( diff --git a/frontend/src/i18n/locales/en/inbox.json b/frontend/src/i18n/locales/en/inbox.json index e44f6fd0..e2138f3c 100644 --- a/frontend/src/i18n/locales/en/inbox.json +++ b/frontend/src/i18n/locales/en/inbox.json @@ -17,6 +17,9 @@ "downloadDone": { "title": "Download ready" }, + "editDone": { + "title": "Edit ready" + }, "maintenance": { "title": "Videos removed", "body_one": "{{count}} saved or playlisted video was removed because it's no longer available on YouTube.", diff --git a/frontend/src/i18n/locales/hu/inbox.json b/frontend/src/i18n/locales/hu/inbox.json index 99b8e747..d9e817a5 100644 --- a/frontend/src/i18n/locales/hu/inbox.json +++ b/frontend/src/i18n/locales/hu/inbox.json @@ -17,6 +17,9 @@ "downloadDone": { "title": "Letöltés kész" }, + "editDone": { + "title": "Vágás kész" + }, "maintenance": { "title": "Videók eltávolítva", "body_one": "{{count}} mentett vagy lejátszási listás videó eltávolítva, mert már nem elérhető a YouTube-on.",