Merge pull request #299 from warmbly/fix/profile-avatar-persistence

Fix profile and workspace avatar changes not persisting after refresh

docs/content/docs/api/reference/account-org.mdx

@@ -470,7 +470,7 @@ Auth: Session only (not available to API keys).
 
 `DELETE /auth/me/avatar`
 
-Clears the profile image. Returns `204 No Content`.
+Clears the profile image and deletes the stored file on a best-effort basis. Returns `204 No Content`.
 
 Auth: Session only (not available to API keys).
 
@@ -1456,7 +1456,7 @@ Auth: Session only (not available to API keys). Requires a selected organization
 
 `DELETE /organization/avatar`
 
-Clears the workspace image. Owner only. Returns `204 No Content`.
+Clears the workspace image and deletes the stored file on a best-effort basis. Owner only. Returns `204 No Content`.
 
 Auth: Session only (not available to API keys). Requires a selected organization (owner only).
 

internal/api/handler/avatar.go

@@ -8,11 +8,14 @@
 // Constants:
 //
 //   - max size: 2 MiB. Anything larger gets a 400.
-//   - accepted MIME: image/png, image/jpeg, image/webp, image/gif.
-//   - object key: avatars/{kind}/{id}-{epoch}.{ext}
+//   - accepted MIME: image/png, image/jpeg (see allowedAvatarMIME).
+//   - object key: avatars/{kind}/{id}-{epoch_ms}-{nonce}.{ext}
 //
 // The epoch suffix forces cache busting on replacement so the
-// browser doesn't keep showing the old avatar at the same URL.
+// browser doesn't keep showing the old avatar at the same URL (the
+// objects are served immutable, so a reused key would never refresh).
+// The previous object is deleted best-effort once the row points at
+// the new one, so replacing or removing an avatar doesn't leak blobs.
 
 package handler
 
@@ -42,6 +45,9 @@ import (
 const (
 	avatarMaxBytes     int64 = 2 * 1024 * 1024
 	avatarMaxDimension       = 1024 // px — reject anything bigger so a phone-camera dump doesn't sneak through
+
+	userAvatarKeyPrefix = "avatars/users/"
+	orgAvatarKeyPrefix  = "avatars/organizations/"
 )
 
 // Intentionally narrow allowlist: only PNG and JPEG. WebP, GIF and
@@ -77,17 +83,23 @@ func (h *Handler) UploadUserAvatar(c *gin.Context) {
 		return
 	}
 
-	key := fmt.Sprintf("avatars/users/%s-%d%s", userID.String(), time.Now().Unix(), ext)
-	url, xerr := putPublicObject(c.Request.Context(), h.Storage, key, bytesRead, mime)
+	ctx := c.Request.Context()
+	previous := h.currentUserAvatarURL(ctx, userID)
+
+	key := avatarObjectKey(userAvatarKeyPrefix, userID, ext)
+	url, xerr := putPublicObject(ctx, h.Storage, key, bytesRead, mime)
 	if xerr != nil {
 		errx.Handle(c, xerr)
 		return
 	}
 
-	if err := h.UserRepo.UpdateAvatar(c.Request.Context(), userID, &url); err != nil {
-		errx.Handle(c, errx.InternalError())
+	// Through the service, not the repo: /auth/me is served from a cached
+	// copy, and only the service drops it.
+	if xerr := h.UserService.UpdateAvatar(ctx, userID, &url); xerr != nil {
+		errx.Handle(c, xerr)
 		return
 	}
+	h.deleteAvatarObject(ctx, previous, userAvatarKeyPrefix, key)
 
 	h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityUser, &userID, nil, map[string]string{"field": "avatar_url"})
 
@@ -103,10 +115,14 @@ func (h *Handler) DeleteUserAvatar(c *gin.Context) {
 		return
 	}
 
-	if err := h.UserRepo.UpdateAvatar(c.Request.Context(), userID, nil); err != nil {
-		errx.Handle(c, errx.InternalError())
+	ctx := c.Request.Context()
+	previous := h.currentUserAvatarURL(ctx, userID)
+
+	if xerr := h.UserService.UpdateAvatar(ctx, userID, nil); xerr != nil {
+		errx.Handle(c, xerr)
 		return
 	}
+	h.deleteAvatarObject(ctx, previous, userAvatarKeyPrefix, "")
 
 	h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityUser, &userID, nil, map[string]string{"field": "avatar_url", "value": "cleared"})
 
@@ -140,17 +156,21 @@ func (h *Handler) UploadOrganizationAvatar(c *gin.Context) {
 		return
 	}
 
-	key := fmt.Sprintf("avatars/organizations/%s-%d%s", orgID.String(), time.Now().Unix(), ext)
-	url, xerr := putPublicObject(c.Request.Context(), h.Storage, key, bytesRead, mime)
+	ctx := c.Request.Context()
+	previous := h.currentOrgAvatarURL(ctx, *orgID)
+
+	key := avatarObjectKey(orgAvatarKeyPrefix, *orgID, ext)
+	url, xerr := putPublicObject(ctx, h.Storage, key, bytesRead, mime)
 	if xerr != nil {
 		errx.Handle(c, xerr)
 		return
 	}
 
-	if err := h.OrgRepo.UpdateAvatar(c.Request.Context(), *orgID, &url); err != nil {
+	if err := h.OrgRepo.UpdateAvatar(ctx, *orgID, &url); err != nil {
 		errx.Handle(c, errx.InternalError())
 		return
 	}
+	h.deleteAvatarObject(ctx, previous, orgAvatarKeyPrefix, key)
 
 	h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityOrganization, orgID, nil, map[string]string{"field": "avatar_url"})
 
@@ -175,14 +195,82 @@ func (h *Handler) DeleteOrganizationAvatar(c *gin.Context) {
 		return
 	}
 
-	if err := h.OrgRepo.UpdateAvatar(c.Request.Context(), *orgID, nil); err != nil {
+	ctx := c.Request.Context()
+	previous := h.currentOrgAvatarURL(ctx, *orgID)
+
+	if err := h.OrgRepo.UpdateAvatar(ctx, *orgID, nil); err != nil {
 		errx.Handle(c, errx.InternalError())
 		return
 	}
+	h.deleteAvatarObject(ctx, previous, orgAvatarKeyPrefix, "")
+
+	// Audited like the upload so teammates' org switcher refreshes live.
+	h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityOrganization, orgID, nil, map[string]string{"field": "avatar_url", "value": "cleared"})
 
 	c.Status(http.StatusNoContent)
 }
 
+// avatarObjectKey builds a key that is unique per upload: the epoch keeps keys
+// sortable, the random nonce keeps two uploads in the same millisecond from
+// sharing an immutably cached URL.
+func avatarObjectKey(prefix string, id uuid.UUID, ext string) string {
+	nonce := strings.ReplaceAll(uuid.NewString(), "-", "")[:12]
+	return fmt.Sprintf("%s%s-%d-%s%s", prefix, id.String(), time.Now().UnixMilli(), nonce, ext)
+}
+
+// currentUserAvatarURL returns the avatar URL stored on the user row, or ""
+// when there is none or the lookup fails (cleanup is best-effort).
+func (h *Handler) currentUserAvatarURL(ctx context.Context, userID uuid.UUID) string {
+	u, xerr := h.UserService.GetUser(ctx, userID)
+	if xerr != nil || u == nil || u.AvatarURL == nil {
+		return ""
+	}
+	return *u.AvatarURL
+}
+
+// currentOrgAvatarURL is the organization counterpart of currentUserAvatarURL.
+func (h *Handler) currentOrgAvatarURL(ctx context.Context, orgID uuid.UUID) string {
+	org, xerr := h.OrganizationService.Get(ctx, orgID)
+	if xerr != nil || org == nil || org.AvatarURL == nil {
+		return ""
+	}
+	return *org.AvatarURL
+}
+
+// deleteAvatarObject removes the object behind a previous avatar URL once the
+// row no longer points at it. Only keys under our own prefix are touched, so
+// an external URL (an OAuth profile picture, say) is left alone, and keepKey
+// guards the freshly written object. Failures are ignored: an orphaned blob
+// is harmless, a failed request after a successful update is not.
+func (h *Handler) deleteAvatarObject(ctx context.Context, previousURL, prefix, keepKey string) {
+	if h.Storage == nil || previousURL == "" {
+		return
+	}
+	key := avatarKeyFromURL(previousURL, prefix)
+	if key == "" || key == keepKey {
+		return
+	}
+	_ = h.Storage.Delete(ctx, key)
+}
+
+// avatarKeyFromURL recovers the object key from a public avatar URL. Both
+// storage backends build the URL differently, but the key always starts with
+// the known prefix, so that is what is looked for.
+func avatarKeyFromURL(url, prefix string) string {
+	idx := strings.Index(url, prefix)
+	if idx < 0 {
+		return ""
+	}
+	key := url[idx:]
+	if q := strings.IndexAny(key, "?#"); q >= 0 {
+		key = key[:q]
+	}
+	if key == prefix || strings.Contains(key, "..") {
+		return ""
+	}
+	return key
+}
+
 func (h *Handler) requireOrgOwner(c *gin.Context, orgID, userID uuid.UUID) *errx.Error {
 	m, err := h.OrgRepo.GetMember(c.Request.Context(), orgID, userID)
 	if err != nil || m == nil {

internal/app/user/onboarding.go

@@ -41,3 +41,16 @@ func (s *userService) UpdateUndoSendSeconds(ctx context.Context, userID uuid.UUI
 
 	return nil
 }
+
+// UpdateAvatar sets (or clears, with nil) the user's avatar URL. It must go
+// through the service so the cached /auth/me copy is dropped; writing the
+// repository directly leaves the old avatar served until UserTTL expires.
+func (s *userService) UpdateAvatar(ctx context.Context, userID uuid.UUID, avatarURL *string) *errx.Error {
+	if err := s.userRepository.UpdateAvatar(ctx, userID, avatarURL); err != nil {
+		return errx.InternalError()
+	}
+
+	s.cache.Del(ctx, getUserKey(userID))
+
+	return nil
+}

internal/app/user/service.go

@@ -16,6 +16,7 @@ type UserService interface {
 	CompleteOnboarding(ctx context.Context, userID uuid.UUID, firstName, lastName, referralSource, role, teamSize string) *errx.Error
 	UpdateProfile(ctx context.Context, userID uuid.UUID, firstName, lastName string) *errx.Error
 	UpdateUndoSendSeconds(ctx context.Context, userID uuid.UUID, seconds int) *errx.Error
+	UpdateAvatar(ctx context.Context, userID uuid.UUID, avatarURL *string) *errx.Error
 }
 
 type userService struct {

web/src/components/app/avatar/AvatarUploader.tsx

@@ -139,7 +139,7 @@ export function AvatarUploader({
                 <div className="text-[11px] text-slate-500 leading-snug">
                     {uploading
                         ? "Uploading…"
-                        : `PNG, JPG, WebP or GIF. We resize to ${AVATAR_OUTPUT_DIMENSION}px before upload.`}
+                        : `PNG or JPG. We resize to ${AVATAR_OUTPUT_DIMENSION}px before upload.`}
                 </div>
                 <div className="flex items-center gap-1 mt-1.5">
                     <button

web/src/lib/api/client/app/organizations/getOrganizations.ts

@@ -13,6 +13,7 @@ interface RawMembership {
         name: string;
         slug?: string;
         avatar?: string;
+        avatar_url?: string | null;
         plan?: string;
         created_at: string;
     };
@@ -36,6 +37,7 @@ export default async function getOrganizations(): Promise<Organization[]> {
             id: r.organization!.id,
             name: r.organization!.name,
             avatar: r.organization!.avatar,
+            avatar_url: r.organization!.avatar_url ?? null,
             plan: r.organization!.plan,
             role: r.role,
             permissions: r.permissions,

web/src/lib/api/hooks/app/avatar/useOrgAvatar.ts

@@ -1,17 +1,49 @@
-import { useMutation, useQueryClient } from "@tanstack/react-query";
+import { useMutation, useQueryClient, type QueryClient } from "@tanstack/react-query";
 import {
     deleteOrganizationAvatar,
     uploadOrganizationAvatar,
 } from "@/lib/api/client/app/avatar/uploadUserAvatar";
+import type Organization from "@/lib/api/models/app/organizations/Organization";
+import { useAppStore } from "@/stores";
+
+// The workspace avatar is read from the persisted zustand org pointer, which
+// is fed by the /organization list. Patch the store and the cached queries
+// with the server's answer right away, then invalidate so the list refetch
+// (and OrgGate's setOrganizations) settles on the same value. Every patch is
+// keyed on the org the mutation targeted, so a workspace switch mid-flight
+// cannot stamp the avatar onto the wrong org.
+function applyOrgAvatar(qc: QueryClient, orgId: string | null, avatarUrl: string | null) {
+    if (orgId) {
+        // Generic: the store rows and the query rows are different org shapes.
+        const patch = <T extends { id: string; avatar_url?: string | null }>(o: T): T =>
+            o.id === orgId ? { ...o, avatar_url: avatarUrl } : o;
+        const store = useAppStore.getState();
+        const current = store.currentOrganization;
+        const list = store.organizations;
+        if (list.some((o) => o.id === orgId)) {
+            // setOrganizations adopts the matching row as the current org.
+            store.setOrganizations(list.map(patch));
+        } else if (current && current.id === orgId) {
+            // List not loaded yet: patch the pointer directly, since
+            // setOrganizations would null it without a matching row.
+            store.setCurrentOrganization({ ...current, avatar_url: avatarUrl });
+        }
+        qc.setQueryData<Organization[]>(["organizations", "list"], (old) => (old ? old.map(patch) : old));
+        qc.setQueryData<Organization>(["organizations", "current"], (old) => (old ? patch(old) : old));
+    }
+    qc.invalidateQueries({ queryKey: ["organizations"] });
+}
+
+// The org a mutation is about is the one selected when it starts, not when
+// it resolves.
+const targetOrgId = () => useAppStore.getState().currentOrganization?.id ?? null;
 
 export function useUploadOrgAvatar() {
     const qc = useQueryClient();
     return useMutation({
         mutationFn: (blob: Blob) => uploadOrganizationAvatar(blob),
-        onSuccess: () => {
-            qc.invalidateQueries({ queryKey: ["organizations"] });
-            qc.invalidateQueries({ queryKey: ["organizations", "current"] });
-        },
+        onMutate: () => ({ orgId: targetOrgId() }),
+        onSuccess: (res, _blob, ctx) => applyOrgAvatar(qc, ctx?.orgId ?? null, res.avatar_url),
     });
 }
 
@@ -19,9 +51,7 @@ export function useDeleteOrgAvatar() {
     const qc = useQueryClient();
     return useMutation({
         mutationFn: () => deleteOrganizationAvatar(),
-        onSuccess: () => {
-            qc.invalidateQueries({ queryKey: ["organizations"] });
-            qc.invalidateQueries({ queryKey: ["organizations", "current"] });
-        },
+        onMutate: () => ({ orgId: targetOrgId() }),
+        onSuccess: (_res, _vars, ctx) => applyOrgAvatar(qc, ctx?.orgId ?? null, null),
     });
 }

web/src/lib/api/hooks/app/avatar/useUserAvatar.ts

@@ -1,16 +1,22 @@
-import { useMutation, useQueryClient } from "@tanstack/react-query";
+import { useMutation, useQueryClient, type QueryClient } from "@tanstack/react-query";
 import {
     deleteUserAvatar,
     uploadUserAvatar,
 } from "@/lib/api/client/app/avatar/uploadUserAvatar";
+import type User from "@/lib/api/models/auth/User";
+
+// Patch the cached /auth/me user with the server's answer so the sidebar and
+// profile page flip immediately, then invalidate so the refetch confirms it.
+function applyUserAvatar(qc: QueryClient, avatarUrl: string | null) {
+    qc.setQueryData<User>(["auth", "me"], (old) => (old ? { ...old, avatar_url: avatarUrl } : old));
+    qc.invalidateQueries({ queryKey: ["auth", "me"] });
+}
 
 export function useUploadUserAvatar() {
     const qc = useQueryClient();
     return useMutation({
         mutationFn: (blob: Blob) => uploadUserAvatar(blob),
-        onSuccess: () => {
-            qc.invalidateQueries({ queryKey: ["auth", "me"] });
-        },
+        onSuccess: (res) => applyUserAvatar(qc, res.avatar_url),
     });
 }
 
@@ -18,8 +24,6 @@ export function useDeleteUserAvatar() {
     const qc = useQueryClient();
     return useMutation({
         mutationFn: () => deleteUserAvatar(),
-        onSuccess: () => {
-            qc.invalidateQueries({ queryKey: ["auth", "me"] });
-        },
+        onSuccess: () => applyUserAvatar(qc, null),
     });
 }

web/src/lib/api/models/app/organizations/Organization.ts

@@ -2,6 +2,8 @@ export default interface Organization {
     id: string
     name: string
     avatar?: string
+    // Public URL of the workspace avatar, null/undefined when none is set.
+    avatar_url?: string | null
     plan?: string
     // Built-in role id or a custom role's name.
     role: string