fix: address code-review findings

- imaging: downscale to 1280px before JPEG quality-stepping so real camera
  photos fit the 200KB cap (was quality-only → rejected typical photos)
- billing: save_prescription/save_checkup_services refuse to modify a PAID
  invoice, so recorded payment can't diverge from the invoice total
- staff: set_staff_role/remove_staff RPCs hold an advisory lock while enforcing
  the last-admin invariant — race-safe vs the prior check-then-act
- imaging delete: remove the object at the row's stored path (DB lookup), not a
  client-supplied path
- invoice PDF: filter deleted=false like every other checkup view
- sign-in: skip rate limit when no client IP is resolvable instead of bucketing
  all requests under a shared "unknown" key
- reminders: bound to a [today-30d, today+7d] window so stale overdue rows
  don't accumulate and bury upcoming ones

app/[locale]/(app)/admin/staff/actions.ts

@@ -2,20 +2,19 @@
 
 /**
  * Staff management Server Actions (admin only). The invite flow remains the
- * create path; here an admin can change a member's role or revoke access
- * (delete the bsk.app_users enrollment — auth.users is untouched).
+ * create path; here an admin can change a member's role or revoke access.
  *
- * Writes use the admin (service_role) client, consistent with the invite flow
- * and the app_users least-privilege posture (authenticated has no direct write).
- * Guards: an admin cannot change/remove their OWN row, and the LAST admin
- * cannot be demoted or removed (avoids locking everyone out).
+ * Mutations go through the set_staff_role / remove_staff RPCs (SECURITY DEFINER,
+ * gated on the caller's current_role() via the USER client) which hold an
+ * advisory lock while enforcing the "keep >= 1 admin" and "no self-change"
+ * invariants — race-safe, unlike a check-then-act in app code.
  */
 
 import { getLocale } from "next-intl/server";
 import { revalidatePath } from "next/cache";
 
 import { getServerSession } from "@/lib/auth/get-server-session";
-import { createSupabaseAdminClient } from "@/lib/supabase/admin";
+import { createSupabaseServerClient } from "@/lib/supabase/server";
 import { isAppRole } from "@/lib/db/roles";
 
 async function revalidateStaff() {
@@ -23,40 +22,18 @@ async function revalidateStaff() {
   revalidatePath(`/${locale}/admin/staff`);
 }
 
-/** True if removing/demoting this user would leave zero admins. */
-async function isLastAdmin(
-  admin: ReturnType<typeof createSupabaseAdminClient>,
-  userId: string,
-): Promise<boolean> {
-  const { data: target } = await admin
-    .from("app_users")
-    .select("role")
-    .eq("user_id", userId)
-    .maybeSingle();
-  if (target?.role !== "admin") return false;
-
-  const { count } = await admin
-    .from("app_users")
-    .select("user_id", { count: "exact", head: true })
-    .eq("role", "admin");
-  return (count ?? 0) <= 1;
-}
-
 export async function updateStaffRoleAction(formData: FormData): Promise<void> {
   const session = await getServerSession();
   if (session?.role !== "admin") return;
 
   const userId = String(formData.get("userId") ?? "");
   const role = String(formData.get("role") ?? "");
-  if (!userId || !isAppRole(role)) return;
-  if (userId === session.user.id) return; // no self role-change
+  if (!userId || !isAppRole(role) || userId === session.user.id) return;
 
-  const admin = createSupabaseAdminClient();
-  if (role !== "admin" && (await isLastAdmin(admin, userId))) return; // keep >=1 admin
-
-  const { error } = await admin.from("app_users").update({ role }).eq("user_id", userId);
+  const supabase = await createSupabaseServerClient();
+  const { error } = await supabase.rpc("set_staff_role", { p_user_id: userId, p_role: role });
   if (!error) {
-    await admin.rpc("log_audit", {
+    await supabase.rpc("log_audit", {
       p_action: "staff.role_change",
       p_entity: "app_users",
       p_entity_id: userId,
@@ -71,14 +48,12 @@ export async function removeStaffAction(formData: FormData): Promise<void> {
   if (session?.role !== "admin") return;
 
   const userId = String(formData.get("userId") ?? "");
-  if (!userId || userId === session.user.id) return; // no self removal
+  if (!userId || userId === session.user.id) return;
 
-  const admin = createSupabaseAdminClient();
-  if (await isLastAdmin(admin, userId)) return; // never remove the last admin
-
-  const { error } = await admin.from("app_users").delete().eq("user_id", userId);
+  const supabase = await createSupabaseServerClient();
+  const { error } = await supabase.rpc("remove_staff", { p_user_id: userId });
   if (!error) {
-    await admin.rpc("log_audit", {
+    await supabase.rpc("log_audit", {
       p_action: "staff.remove",
       p_entity: "app_users",
       p_entity_id: userId,

app/[locale]/(app)/checkups/[id]/imaging/actions.ts

@@ -81,6 +81,17 @@ export async function deleteImageAction(
 
   const supabase = await createSupabaseServerClient();
 
+  // Look up the row's ACTUAL storage_path (scoped by id + checkup_id) — never
+  // trust the client-supplied path, so a scripted call can't remove another
+  // checkup's object.
+  const { data: row } = await supabase
+    .from("checkup_images")
+    .select("storage_path")
+    .eq("id", parsed.data.imageId)
+    .eq("checkup_id", parsed.data.checkupId)
+    .maybeSingle();
+  if (!row) return { status: "error", message: t("errorGeneric") };
+
   const { error: updateError } = await supabase
     .from("checkup_images")
     .update({ deleted: true })
@@ -89,9 +100,9 @@ export async function deleteImageAction(
   if (updateError) return { status: "error", message: t("errorGeneric") };
 
   // Best-effort object removal — metadata is already soft-deleted so the
-  // gallery hides it regardless; a future retention sweep (Phase 7) reconciles
-  // any orphaned object left behind by a failed removal here.
-  await supabase.storage.from(CHECKUP_MEDIA_BUCKET).remove([parsed.data.storagePath]);
+  // gallery hides it regardless; the nightly retention sweep reconciles any
+  // orphaned object left behind by a failed removal here.
+  await supabase.storage.from(CHECKUP_MEDIA_BUCKET).remove([row.storage_path]);
 
   await supabase.rpc("log_audit", {
     p_action: "image.delete",

app/[locale]/(app)/checkups/[id]/imaging/image-capture.tsx

@@ -25,6 +25,22 @@ import { CHECKUP_MEDIA_BUCKET, MAX_IMAGE_BYTES, buildStoragePath } from "@/lib/i
 import { recordImageAction } from "./actions";
 
 const QUALITY_STEPS = [0.9, 0.8, 0.7, 0.6, 0.5, 0.4] as const;
+// Downscale the longest side to this before quality-stepping — a full-res phone
+// photo (e.g. 4000×3000) never fits 200KB on JPEG quality alone, so we must
+// reduce resolution first. 1280px keeps ultrasound/clinic detail legible.
+const MAX_DIMENSION = 1280;
+
+/** Draw a source (video frame or image) onto a canvas scaled to fit MAX_DIMENSION. */
+function makeScaledCanvas(source: CanvasImageSource, w: number, h: number): HTMLCanvasElement | null {
+  const scale = Math.min(1, MAX_DIMENSION / Math.max(w, h));
+  const canvas = document.createElement("canvas");
+  canvas.width = Math.max(1, Math.round(w * scale));
+  canvas.height = Math.max(1, Math.round(h * scale));
+  const ctx = canvas.getContext("2d");
+  if (!ctx) return null;
+  ctx.drawImage(source, 0, 0, canvas.width, canvas.height);
+  return canvas;
+}
 
 // getUserMedia support never changes after mount, so there's nothing to
 // subscribe to — this is purely a way to read a browser-only value without
@@ -55,7 +71,6 @@ export function ImageCapture({ checkupId }: { checkupId: number }) {
   const router = useRouter();
 
   const videoRef = useRef<HTMLVideoElement | null>(null);
-  const canvasRef = useRef<HTMLCanvasElement | null>(null);
   const streamRef = useRef<MediaStream | null>(null);
 
   // Browser-only feature detection without an Effect: getServerSnapshot
@@ -138,12 +153,11 @@ export function ImageCapture({ checkupId }: { checkupId: number }) {
   function snapshot() {
     const video = videoRef.current;
     if (!video || !video.videoWidth) return;
-    const canvas = canvasRef.current ?? document.createElement("canvas");
-    canvas.width = video.videoWidth;
-    canvas.height = video.videoHeight;
-    const ctx = canvas.getContext("2d");
-    if (!ctx) return;
-    ctx.drawImage(video, 0, 0, canvas.width, canvas.height);
+    const canvas = makeScaledCanvas(video, video.videoWidth, video.videoHeight);
+    if (!canvas) {
+      setError(t("errorGeneric"));
+      return;
+    }
     void uploadFromCanvas(canvas);
   }
 
@@ -155,16 +169,12 @@ export function ImageCapture({ checkupId }: { checkupId: number }) {
     const img = new Image();
     const objectUrl = URL.createObjectURL(file);
     img.onload = () => {
-      const canvas = document.createElement("canvas");
-      canvas.width = img.naturalWidth;
-      canvas.height = img.naturalHeight;
-      const ctx = canvas.getContext("2d");
+      const canvas = makeScaledCanvas(img, img.naturalWidth, img.naturalHeight);
       URL.revokeObjectURL(objectUrl);
-      if (!ctx) {
+      if (!canvas) {
         setError(t("errorGeneric"));
         return;
       }
-      ctx.drawImage(img, 0, 0);
       void uploadFromCanvas(canvas);
     };
     img.onerror = () => {
@@ -220,8 +230,6 @@ export function ImageCapture({ checkupId }: { checkupId: number }) {
         {uploading && <p className="text-muted-foreground text-sm">{t("uploading")}</p>}
       </div>
 
-      <canvas ref={canvasRef} className="hidden" />
-
       {error && (
         <p className="text-destructive text-sm" role="alert">
           {error}

app/[locale]/(app)/checkups/[id]/invoice/route.ts

@@ -23,7 +23,12 @@ export async function GET(_req: Request, { params }: { params: Promise<{ locale:
   const supabase = await createSupabaseServerClient();
 
   const [{ data: c }, { data: clinic }] = await Promise.all([
-    supabase.from("checkups").select("id, customer_id, queue_number, checkup_date").eq("id", checkupId).maybeSingle(),
+    supabase
+      .from("checkups")
+      .select("id, customer_id, queue_number, checkup_date")
+      .eq("id", checkupId)
+      .eq("deleted", false)
+      .maybeSingle(),
     supabase.from("clinic_settings").select("name, address, phone").eq("id", true).maybeSingle(),
   ]);
   if (!c) return new Response("Not found", { status: 404 });

app/[locale]/(app)/reminders/page.tsx

@@ -16,12 +16,15 @@ export default async function RemindersPage({ params }: { params: Promise<{ loca
   const now = new Date();
   const today = fmt.format(now);
   const horizon = fmt.format(new Date(now.getTime() + 7 * 86_400_000));
+  // Lower bound so ancient overdue rows drop off instead of accumulating forever
+  // and burying this-week reminders past the row limit.
+  const floor = fmt.format(new Date(now.getTime() - 30 * 86_400_000));
 
   const supabase = await createSupabaseServerClient();
   const { data: checkups } = await supabase
     .from("checkups")
     .select("id, customer_id, recheck_date")
-    .not("recheck_date", "is", null)
+    .gte("recheck_date", floor)
     .lte("recheck_date", horizon)
     .eq("deleted", false)
     .order("recheck_date", { ascending: true })

app/[locale]/(auth)/sign-in/actions.ts

@@ -61,17 +61,25 @@ export async function signInAction(
   // XFF hop (appended by the trusted proxy).
   const hdrs = await headers();
   const xff = hdrs.get("x-forwarded-for");
-  const ip = hdrs.get("x-real-ip")?.trim() || xff?.split(",").at(-1)?.trim() || "unknown";
-  try {
-    const { success: withinLimit } = await signInLimiter.limit(ip);
-    if (!withinLimit) {
-      return { status: "error", fieldErrors: {}, formError: t("tooManyAttempts") };
+  const ip = hdrs.get("x-real-ip")?.trim() || xff?.split(",").at(-1)?.trim() || null;
+  // Only rate-limit when a real client IP is resolvable. If neither header is
+  // set (misconfigured proxy / non-Vercel host), skip rather than bucket every
+  // request under a shared "unknown" key — which would lock out the whole clinic
+  // after 5 attempts/min. Logged so the missing-IP case is visible.
+  if (!ip) {
+    console.warn("[sign-in] no client IP header; skipping rate limit");
+  } else {
+    try {
+      const { success: withinLimit } = await signInLimiter.limit(ip);
+      if (!withinLimit) {
+        return { status: "error", fieldErrors: {}, formError: t("tooManyAttempts") };
+      }
+    } catch (err) {
+      // Shared Redis unavailable: fail OPEN so a sibling app's outage can't lock
+      // doctors out of the clinic. Logged so the gap in brute-force protection
+      // is alertable rather than silent.
+      console.warn("[sign-in] rate limiter unavailable, failing open:", err);
     }
-  } catch (err) {
-    // Shared Redis unavailable: fail OPEN so a sibling app's outage can't lock
-    // doctors out of the clinic. Logged so the gap in brute-force protection
-    // is alertable rather than silent.
-    console.warn("[sign-in] rate limiter unavailable, failing open:", err);
   }
 
   const supabase = await createSupabaseServerClient();

supabase/migrations/20260725140000_bsk_review_fixes.sql

@@ -0,0 +1,153 @@
+-- BSK — code-review fixes.
+--
+-- (2) Lock prescriptions/services once an invoice is paid: save_prescription /
+--     save_checkup_services now refuse to modify a checkup whose medicine_orders
+--     row is 'paid', so a recorded payment can never diverge from the invoice.
+-- (4) Race-safe staff mutations: set_staff_role / remove_staff hold an advisory
+--     lock while checking the "keep >= 1 admin" invariant, so two concurrent
+--     demote/remove operations can't both pass the guard and orphan the clinic.
+
+-- ─── (2) save_prescription — refuse when paid ───────────────────────────────
+CREATE OR REPLACE FUNCTION bsk.save_prescription(p_checkup_id bigint, p_items jsonb)
+  RETURNS void
+  LANGUAGE plpgsql
+  VOLATILE
+  SECURITY DEFINER
+  SET search_path = bsk, pg_catalog
+AS $$
+DECLARE
+  v_role bsk.app_role := bsk.current_role();
+BEGIN
+  IF v_role IS NULL OR v_role NOT IN ('admin', 'receptionist', 'doctor', 'nurse') THEN
+    RAISE EXCEPTION 'not authorized to save a prescription';
+  END IF;
+
+  IF EXISTS (SELECT 1 FROM bsk.medicine_orders WHERE checkup_id = p_checkup_id AND payment_status = 'paid') THEN
+    RAISE EXCEPTION 'cannot modify a paid invoice';
+  END IF;
+
+  DELETE FROM bsk.order_items WHERE checkup_id = p_checkup_id;
+
+  INSERT INTO bsk.order_items (checkup_id, medicine_id, quantity, dosage, unit_price, line_total, notes)
+  SELECT
+    p_checkup_id,
+    (elem->>'medicine_id')::bigint,
+    (elem->>'quantity')::integer,
+    NULLIF(elem->>'dosage', ''),
+    m.sale_price,
+    m.sale_price * (elem->>'quantity')::integer,
+    NULLIF(elem->>'notes', '')
+  FROM jsonb_array_elements(COALESCE(p_items, '[]'::jsonb)) AS elem
+  JOIN bsk.medicines m ON m.id = (elem->>'medicine_id')::bigint;
+
+  INSERT INTO bsk.medicine_orders (checkup_id) VALUES (p_checkup_id)
+  ON CONFLICT (checkup_id) DO NOTHING;
+END
+$$;
+
+-- ─── (2) save_checkup_services — refuse when paid ───────────────────────────
+CREATE OR REPLACE FUNCTION bsk.save_checkup_services(p_checkup_id bigint, p_items jsonb)
+  RETURNS void
+  LANGUAGE plpgsql
+  VOLATILE
+  SECURITY DEFINER
+  SET search_path = bsk, pg_catalog
+AS $$
+DECLARE
+  v_role bsk.app_role := bsk.current_role();
+BEGIN
+  IF v_role IS NULL OR v_role NOT IN ('admin', 'receptionist', 'doctor', 'nurse') THEN
+    RAISE EXCEPTION 'not authorized to save checkup services';
+  END IF;
+
+  IF EXISTS (SELECT 1 FROM bsk.medicine_orders WHERE checkup_id = p_checkup_id AND payment_status = 'paid') THEN
+    RAISE EXCEPTION 'cannot modify a paid invoice';
+  END IF;
+
+  DELETE FROM bsk.checkup_services WHERE checkup_id = p_checkup_id;
+
+  INSERT INTO bsk.checkup_services (checkup_id, service_id, quantity, unit_price, line_total)
+  SELECT
+    p_checkup_id,
+    (elem->>'service_id')::bigint,
+    (elem->>'quantity')::integer,
+    s.price,
+    s.price * (elem->>'quantity')::integer
+  FROM jsonb_array_elements(COALESCE(p_items, '[]'::jsonb)) AS elem
+  JOIN bsk.services s ON s.id = (elem->>'service_id')::bigint;
+END
+$$;
+
+-- ─── (4) Race-safe staff mutations ──────────────────────────────────────────
+-- Both take pg_advisory_xact_lock so the last-admin check-then-act is serialized.
+-- SECURITY DEFINER (write app_users) but gated on the CALLER's current_role().
+
+CREATE OR REPLACE FUNCTION bsk.set_staff_role(p_user_id uuid, p_role bsk.app_role)
+  RETURNS void
+  LANGUAGE plpgsql
+  VOLATILE
+  SECURITY DEFINER
+  SET search_path = bsk, pg_catalog
+AS $$
+DECLARE
+  v_target_role bsk.app_role;
+BEGIN
+  IF bsk.current_role() <> 'admin' THEN
+    RAISE EXCEPTION 'not authorized to change staff roles';
+  END IF;
+  IF p_user_id = auth.uid() THEN
+    RAISE EXCEPTION 'cannot change your own role';
+  END IF;
+
+  PERFORM pg_advisory_xact_lock(hashtext('bsk:staff')::bigint);
+
+  SELECT role INTO v_target_role FROM bsk.app_users WHERE user_id = p_user_id;
+  IF v_target_role IS NULL THEN
+    RAISE EXCEPTION 'user not enrolled';
+  END IF;
+
+  -- Demoting the last admin is forbidden.
+  IF v_target_role = 'admin' AND p_role <> 'admin'
+     AND (SELECT count(*) FROM bsk.app_users WHERE role = 'admin') <= 1 THEN
+    RAISE EXCEPTION 'cannot demote the last admin';
+  END IF;
+
+  UPDATE bsk.app_users SET role = p_role WHERE user_id = p_user_id;
+END
+$$;
+
+CREATE OR REPLACE FUNCTION bsk.remove_staff(p_user_id uuid)
+  RETURNS void
+  LANGUAGE plpgsql
+  VOLATILE
+  SECURITY DEFINER
+  SET search_path = bsk, pg_catalog
+AS $$
+DECLARE
+  v_target_role bsk.app_role;
+BEGIN
+  IF bsk.current_role() <> 'admin' THEN
+    RAISE EXCEPTION 'not authorized to remove staff';
+  END IF;
+  IF p_user_id = auth.uid() THEN
+    RAISE EXCEPTION 'cannot remove yourself';
+  END IF;
+
+  PERFORM pg_advisory_xact_lock(hashtext('bsk:staff')::bigint);
+
+  SELECT role INTO v_target_role FROM bsk.app_users WHERE user_id = p_user_id;
+  IF v_target_role IS NULL THEN
+    RETURN; -- already gone
+  END IF;
+
+  IF v_target_role = 'admin'
+     AND (SELECT count(*) FROM bsk.app_users WHERE role = 'admin') <= 1 THEN
+    RAISE EXCEPTION 'cannot remove the last admin';
+  END IF;
+
+  DELETE FROM bsk.app_users WHERE user_id = p_user_id;
+END
+$$;
+
+GRANT EXECUTE ON FUNCTION bsk.set_staff_role(uuid, bsk.app_role) TO authenticated;
+GRANT EXECUTE ON FUNCTION bsk.remove_staff(uuid) TO authenticated;

types/supabase-bsk.ts

@@ -512,6 +512,14 @@ export type Database = {
         };
         Returns: number;
       };
+      set_staff_role: {
+        Args: { p_user_id: string; p_role: Database["bsk"]["Enums"]["app_role"] };
+        Returns: undefined;
+      };
+      remove_staff: {
+        Args: { p_user_id: string };
+        Returns: undefined;
+      };
       save_prescription: {
         Args: { p_checkup_id: number; p_items: Json };
         Returns: undefined;