Phase A: add canonical avatar resolver#680
Conversation
samxu01
left a comment
There was a problem hiding this comment.
APPROVE — code is right. One product consequence needs an explicit decision before merge (not a code defect).
Verified against the Phase A spec. avatarService.ts is exactly the contract asked for: absolute Commonly origins collapse to /api/uploads/<id>, already-relative and data: pass through, 'default' and empty → null, external CDN URLs are preserved (right call — we can't invent a local object key for them), and resolveAvatarUrls batches to one query with missing ids as null. The test table covers all of it. No migration, no schema change — reversible by revert, as specified.
The two behavioral fixes are the heart of it and both are correct per CLAUDE.md rule 8:
agentIdentityService.ts:395seedsprofilePicturefrom the package icon only when creating a new identity — reinstall preserves a customized avatar (tested).templates.tsdrops bothUser.updateManycalls, so editing package metadata no longer reaches into live identity rows.
I also chased the one thing that looked like a Critical and cleared it: handleUpload now returns a relative URL for every upload, not just avatars (post images, chat attachments). That's a contract change wider than "avatar write path" — but it's safe, because every render path already normalizes: normalizeUploadUrl (apiBaseUrl.ts:37) resolves /api/uploads/... against the API origin and is already called by PostFeed, ChatRoom, Thread, AgentCard, and avatarUtils; v2 attachments already build relative paths and sign them (V2MessageBubble.tsx:250). I found no server-side path that ships an upload URL to an external platform (Discord's att.url values are inbound). Old absolute URLs still render, so both shapes coexist. The new uploads.post.test.js assertion pinning the response to relative-and-not-https:// is the right guard. Deleting the cf-visitor scheme-sniffing block is a real win — that whole class of Mixed-Content bug is now unreachable.
Important — for the merger, not the author
Removing the templates.ts → User.updateMany sync is correct, and it removes the only path that currently exists for changing a live agent's avatar. Editing a template icon was the (accidental) mechanism people were using. After this merges, a deployed agent's avatar can only be changed by authenticating as that agent against updateProfile, or by a direct DB write — there is no UI. That's exactly the gap #570 is meant to fill, and this PR is its foundation, so the sequencing is intentional. But it is a real, user-visible capability regression in the window between this merge and #570.
@xcjsam — your call, and it's a genuine fork: (a) merge as-is and treat #570 as the immediate next item (my recommendation — the old path was a bug that overwrote identity, and preserving it would preserve the thing we're fixing), or (b) ask for a small admin-only PATCH /api/agents/:id/avatar in this PR to bridge the window. Don't merge without picking one.
Nits (non-blocking)
normalizeAvatarUrlmaps'default'→null, butagentIdentityServicestill writes the literal string'default'as its fallback (:395) and the User schema defaults to it. Two sentinels for "no avatar" now coexist. Readers are consistent (resolveAvatarUrlmaps'default'→null), so nothing breaks — but Phase C is the moment to collapse them to one.authController/userControllerchangedif (profilePicture)→if (profilePicture !== undefined), so an empty string now clears an avatar instead of being ignored. That's almost certainly intended (it's how #570 will clear one) — just naming it so it's a decision, not an accident.
b08e315 to
e98baaf
Compare
Summary
Phase A of #569. This PR adds the canonical avatar API and normalizes new writes without migrating or mutating existing data.
normalizeAvatarUrl,resolveAvatarUrl, and batchedresolveAvatarUrls/api/uploads/...paths at profile, upload, template, registry seed, and agent identity creation boundariesiconUrlseparate from participant identity: package icons seed only newly created agent users, while reinstall preserves customizedUser.profilePictureMigration safety
Verification
git diff --check: passedExisting baseline findings
tsc --noEmitstill reports the three pre-existing errors inV2GithubPrCard.tsxandV2MessageBubble.tsxNo visual or layout rules changed, so the v2 compatibility check is a DOM/render assertion plus a production build rather than a browser-layout check.