feat: add local image upload for character avatars#3
Conversation
- Add ImageUploader component with drag-and-drop for images/videos - Add Assets tab to CharacterPanel for per-emotion uploads - Add getBinaryFile() to diskStorage for retrieving uploaded images - Add characterAssetUpload.ts utility (fileToBase64, uploadCharacterAsset, getCharacterAssetUrl) - Extend localFileApi for binary content stored as base64 - Add documentation: AGENTS.md, AVATAR.md, TTS.md
Reviewer's GuideImplements local image/video upload support for character avatars by introducing a reusable ImageUploader component, a character asset upload helper using the existing binary storage layer, a new Assets tab in CharacterEditor, and documentation updates for avatars and TTS, while preserving URL-based configuration and emotion handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a character asset upload system, enabling users to upload and manage images and videos for character avatars and expressions. Key additions include an ImageUploader component, a tabbed interface in the CharacterPanel, and utility functions for binary file storage. Technical feedback highlights several critical issues: the performance impact of using base64 data URLs for video previews, a potential application crash caused by using the spread operator on large binary buffers, and a flawed base64 detection heuristic that could lead to data corruption in text files. Additionally, improving user feedback for failed uploads was recommended.
| const result = await getBinaryFile(path); | ||
| if (result) { | ||
| return `data:${result.mimeType};base64,${result.base64}`; | ||
| } |
There was a problem hiding this comment.
Fetching the entire binary file and converting it to a base64 data URL for previewing is extremely inefficient, especially for video assets. This can lead to significant memory pressure and UI jank. Since the assets are served by the local session-data API, you should ideally return a direct URL to the file instead of its base64 representation.
| const arrayBuffer = await blob.arrayBuffer(); | ||
| const bytes = new Uint8Array(arrayBuffer); | ||
| const binary = String.fromCharCode(...bytes); | ||
| const base64 = btoa(binary); |
There was a problem hiding this comment.
The use of the spread operator ...bytes inside String.fromCharCode will cause a Maximum call stack size exceeded error for files larger than a few dozen kilobytes (the exact limit depends on the browser's stack size). This will crash the application when trying to preview uploaded videos or high-resolution images. Using FileReader is a safer way to convert a Blob to a base64 string.
const blob = await res.blob();
const base64 = await new Promise<string>((resolve, reject) => {
const reader = new FileReader();
reader.onload = () => {
const result = reader.result as string;
resolve(result.split(',')[1]);
};
reader.onerror = () => reject(new Error('Failed to read binary file'));
reader.readAsDataURL(blob);
});| } catch (err) { | ||
| console.warn('Upload failed:', err); | ||
| console.error('Failed to upload asset:', err); | ||
| } finally { |
There was a problem hiding this comment.
When an asset upload fails, the error is logged to the console but the user is not notified in the UI. The component remains in a state where the "Uploading..." indicator simply disappears, which provides no feedback to the user that something went wrong.
| if (mimeType && typeof content === 'string' && content.length > 0) { | ||
| const isDataUrl = content.startsWith('data:'); | ||
| const isBase64 = /^[A-Za-z0-9+/=]+$/.test(content.replace(/\s/g, '')); | ||
| if (!isDataUrl && isBase64 && content.length % 4 === 0) { | ||
| content = `data:${mimeType};base64,${content}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
The heuristic used to detect base64 content is too broad. Any plain text string that happens to be a multiple of 4 in length and contains only alphanumeric characters (e.g., "Test" or "Data") will be incorrectly identified as base64 and wrapped in a data URL, corrupting the file content when read.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/webuiapps/src/lib/diskStorage.ts" line_range="131-140" />
<code_context>
+ * Read a binary file (e.g. image) and return as base64 string.
+ * Returns { base64, mimeType } or null if not found.
+ */
+export async function getBinaryFile(
+ filePath: string,
+): Promise<{ base64: string; mimeType: string } | null> {
+ try {
+ const res = await fetch(apiUrl(filePath));
+ if (!res.ok) return null;
+ const mimeType = res.headers.get('Content-Type') || 'application/octet-stream';
+ const blob = await res.blob();
+ const arrayBuffer = await blob.arrayBuffer();
+ const bytes = new Uint8Array(arrayBuffer);
+ const binary = String.fromCharCode(...bytes);
+ const base64 = btoa(binary);
+ return { base64, mimeType };
</code_context>
<issue_to_address>
**issue (bug_risk):** Building a binary string with `String.fromCharCode(...bytes)` risks stack overflows and is inefficient for large files.
For larger assets this spread call can hit the engine’s max-argument limit and allocate a huge argument list, causing failures or severe slowdowns. Consider chunking or a streaming/base64 helper instead, e.g.:
```ts
const bytes = new Uint8Array(arrayBuffer);
let binary = '';
const chunkSize = 0x8000;
for (let i = 0; i < bytes.length; i += chunkSize) {
binary += String.fromCharCode.apply(null, bytes.subarray(i, i + chunkSize) as unknown as number[]);
}
const base64 = btoa(binary);
```
or use a browser/helper API that handles large blobs safely.
</issue_to_address>
### Comment 2
<location path="apps/webuiapps/src/lib/characterAssetUpload.ts" line_range="53-56" />
<code_context>
+ * Upload a character asset (image or video) and return the storage path.
+ * Works in production mode via diskStorage API.
+ */
+export async function uploadCharacterAsset(
+ characterId: string,
+ emotion: string,
+ file: File,
+ _type: 'image' | 'video',
+): Promise<string> {
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The `_type` parameter is unused and may confuse readers about its purpose.
Since the extension is derived solely from `file.type`, `_type` adds no value and can mislead callers about enforced constraints. Please either remove `_type` or use it to validate `file.type` (e.g., ensuring only video uploads when a video is expected).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| export async function uploadCharacterAsset( | ||
| characterId: string, | ||
| emotion: string, | ||
| file: File, |
There was a problem hiding this comment.
nitpick (bug_risk): The _type parameter is unused and may confuse readers about its purpose.
Since the extension is derived solely from file.type, _type adds no value and can mislead callers about enforced constraints. Please either remove _type or use it to validate file.type (e.g., ensuring only video uploads when a video is expected).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 164b8237e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const type = isVid ? 'video' : 'image'; | ||
| const path = await uploadCharacterAsset(characterId, emotion, file, type); | ||
| await resolvePreviewUrl(path); | ||
| onUpload(path); |
There was a problem hiding this comment.
When a user uploads a local avatar/emotion asset, this passes the raw storage path (for example /characters/<id>/emotions/happy.png) into the character config. I checked the avatar render path in ChatSubComponents.tsx: resolveEmotionMedia() returns that string unchanged and the component uses it directly as the <img>/<video> src, so outside this uploader preview the browser requests /characters/... instead of /api/session-data?path=... and the saved uploaded avatar does not render. Store/return an API URL (as imageGenTools.buildFileUrl() does) or make the avatar renderer resolve these storage paths before rendering.
Useful? React with 👍 / 👎.
| const mimeType = res.headers.get('Content-Type') || 'application/octet-stream'; | ||
| const blob = await res.blob(); | ||
| const arrayBuffer = await blob.arrayBuffer(); | ||
| const bytes = new Uint8Array(arrayBuffer); |
There was a problem hiding this comment.
For normal uploaded images and especially videos, String.fromCharCode(...bytes) can exceed the JavaScript argument/stack limit because the uploader reads the whole binary file into a Uint8Array and then spreads every byte as a function argument. In that case getBinaryFile() catches the RangeError and returns null, so the preview falls back to the raw /characters/... path and breaks for otherwise valid larger uploads. Convert in chunks or use a Blob/FileReader data URL path instead of spreading the whole file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds local (drag-and-drop / file picker) upload support for character avatar assets in the CharacterPanel, backed by session-data binary storage helpers, and introduces new internal docs for avatar/TTS architecture.
Changes:
- Adds an
ImageUploaderUI and an “Assets” tab to upload per-emotion images/videos (and base avatar) into diskStorage-backed paths. - Extends the storage layer with
getBinaryFile()and addscharacterAssetUploadhelpers for upload + preview. - Adds new documentation files (
AVATAR.md,TTS.md) describing avatar and TTS-related architecture.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
apps/webuiapps/src/components/ChatPanel/CharacterPanel.tsx |
Adds tabs and integrates ImageUploader for base avatar + emotion asset management. |
apps/webuiapps/src/components/ChatPanel/ImageUploader.tsx |
New drag/drop uploader with image/video preview and removal. |
apps/webuiapps/src/lib/characterAssetUpload.ts |
Adds file→base64 conversion and upload/preview helpers for character assets. |
apps/webuiapps/src/lib/diskStorage.ts |
Adds getBinaryFile() to read binary content as base64 + MIME type. |
apps/webuiapps/src/lib/localFileApi.ts |
Wraps base64 content into data: URLs when a MIME type is present (mock mode). |
apps/webuiapps/src/components/ChatPanel/panel.module.scss |
Adds styling for the new asset upload grid/slots. |
AVATAR.md |
New internal documentation for the avatar/expression system. |
TTS.md |
New internal documentation summarizing lack of TTS support and integration points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mimeType = res.headers.get('Content-Type') || 'application/octet-stream'; | ||
| const blob = await res.blob(); | ||
| const arrayBuffer = await blob.arrayBuffer(); | ||
| const bytes = new Uint8Array(arrayBuffer); | ||
| const binary = String.fromCharCode(...bytes); | ||
| const base64 = btoa(binary); | ||
| return { base64, mimeType }; |
| try { | ||
| const type = isVid ? 'video' : 'image'; | ||
| const path = await uploadCharacterAsset(characterId, emotion, file, type); | ||
| await resolvePreviewUrl(path); | ||
| onUpload(path); | ||
| } catch (err) { |
| {imageUrl && ( | ||
| <div className={styles.avatarPreview}> | ||
| <img src={imageUrl} alt={name} className={styles.avatarImg} /> | ||
| </div> |
| {emotionImages[e] && | ||
| (/\.(mp4|webm|mov|ogg)(\?|$)/i.test(emotionImages[e]) ? ( | ||
| <video | ||
| src={emotionImages[e]} | ||
| className={styles.emotionThumb} | ||
| autoPlay | ||
| loop | ||
| muted | ||
| playsInline | ||
| /> | ||
| ) : ( | ||
| <img src={emotionImages[e]} alt={e} className={styles.emotionThumb} /> | ||
| ))} |
| /** | ||
| * Get the display URL for a character asset. | ||
| * Returns data URL for local files, original path for external URLs. | ||
| */ | ||
| export async function getCharacterAssetUrl(path: string): Promise<string> { | ||
| if (isExternalUrl(path)) { | ||
| return path; | ||
| } | ||
| const result = await getBinaryFile(path); | ||
| if (result) { | ||
| return `data:${result.mimeType};base64,${result.base64}`; | ||
| } | ||
| return path; |
| ### Type Definitions | ||
|
|
||
| ```typescript | ||
| // /home/niya/github/OpenRoom/apps/webuiapps/src/lib/characterManager.ts |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| 'image/svg+xml': 'svg', | ||
| 'video/mp4': 'mp4', | ||
| 'video/webm': 'webm', | ||
| 'video/ogg': 'ogv', |
There was a problem hiding this comment.
🔥 The Roast: Your MIME_TO_EXT map rolls out the red carpet for .mov files in the regex, then slams the door here by demoting them to .bin. It's like inviting someone to a party and seating them in the dumpster.
🩹 The Fix:
| 'video/ogg': 'ogv', | |
| 'video/ogg': 'ogv', | |
| 'video/quicktime': 'mov', |
📏 Severity: suggestion
|
|
||
| useEffect(() => { | ||
| if (currentUrl) { | ||
| resolvePreviewUrl(currentUrl); |
There was a problem hiding this comment.
🔥 The Roast: Firing an async resolvePreviewUrl inside useEffect without cleanup is like starting a microwave and leaving the house. If currentUrl changes rapidly, the first resolve can come back after the second one and overwrite state with stale data. Enjoy your cold pizza... I mean, wrong preview.
🩹 The Fix: Track the latest URL with a ref or a cancellation flag, and bail out if the effect has been cleaned up before the async work finishes.
📏 Severity: warning
| const path = await uploadCharacterAsset(characterId, emotion, file, type); | ||
| await resolvePreviewUrl(path); | ||
| onUpload(path); | ||
| } catch (err) { |
There was a problem hiding this comment.
🔥 The Roast: Upload fails? Let's log it to the console twice and call it a day. The user gets zero feedback — their file just evaporates back into the dropzone void. Silent failures are the ninjas of bad UX.
🩹 The Fix: Surface the error to the user with inline text or a toast. Also pick one log level — console.error is sufficient; console.warn is just making the console feel important.
📏 Severity: suggestion
| emotion={e} | ||
| currentUrl={emotionImages[e]} | ||
| onUpload={(url) => updateEmotionImage(e, url)} | ||
| onRemove={() => { |
There was a problem hiding this comment.
🔥 The Roast: Clicking "Remove" clears the URL from React state but leaves the actual file rotting on disk like digital tumbleweed. Upload 20 assets, remove 19 of them, and you've got a graveyard of orphaned files that nobody cleans up. Your storage bill called; it wants a word.
🩹 The Fix: Call deleteFilesByPaths (or the equivalent storage delete API) when removing an uploaded asset, or schedule a cleanup routine that purges unreferenced character assets.
📏 Severity: suggestion
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The video support is actually wired up end-to-end now — 💀 Worst part: 📊 Overall: Like a mechanic who fixed the engine, replaced the tires, and then left the parking brake on. Almost there — just tidy up the state/delete ordering and the stale preview guard, and this thing is ready to ship. Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by kimi-for-coding · 1,339,941 tokens |
…ditions - Fix getBinaryFile RangeError with chunked base64 conversion - Fix avatar path resolution: CharacterAvatar and list view now resolve local storage paths to data URLs before rendering - Add path sanitization to prevent path injection in upload paths - Fix base64 detection regex in localFileApi (use mimeType as source of truth) - Fix ImageUploader race condition with expectedUrlRef tracking - Add video/quicktime MIME type mapping
| result += String.fromCharCode(...chunk); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🔥 The Roast: The function is called uint8ToBase64 but it returns a binary string, not base64. You chunked the String.fromCharCode to avoid the stack overflow (good!), but you forgot to btoa the result. So getBinaryFile returns { base64: '<binary gibberish>' }, and the data URL becomes data:image/png;base64,<binary gibberish>. That's not a data URL — that's a digital exorcism. Every uploaded asset is now a broken image.
🩹 The Fix:
| } | |
| return btoa(result); |
📏 Severity: critical
| import { getCharacterAssetUrl } from '@/lib/characterAssetUpload'; | ||
|
|
||
| export function useResolvedAssetUrl(url: string | undefined): string | undefined { | ||
| const [resolvedUrl, setResolvedUrl] = useState<string | undefined>(url); |
There was a problem hiding this comment.
🔥 The Roast: Initializing useState with the raw url means on first render, a local path like /characters/abc/emotions/happy.png is passed straight to <img src>. The browser immediately fires a request for that path, gets a 404, and renders a broken image for one glorious frame. Then the effect resolves it to a data URL and fixes it. It's a strobe light of shame.
🩹 The Fix: Initialize with undefined for local paths so the component shows a placeholder until resolution completes. Or initialize unconditionally with undefined and let the effect handle all URLs on mount.
📏 Severity: warning
| className={styles.avatarImage} | ||
| style={layerStyle} | ||
| src={layer.url} | ||
| src={resolvedUrls[layer.url] || layer.url} |
There was a problem hiding this comment.
🔥 The Roast: resolvedUrls[layer.url] || layer.url — the fallback to layer.url is a 404 waiting to happen. While getCharacterAssetUrl is off fetching the data URL, the browser is already trying to load the raw storage path. For external URLs this is fine, but for local paths it's a guaranteed network error on every initial render. You're papercutting your own error logs.
🩹 The Fix: Seed resolvedUrls synchronously with external URLs on initial state (or render), and use an empty string '' as the fallback for unresolved local paths so the browser doesn't fire a doomed request.
📏 Severity: warning
| currentUrl.startsWith('https://') || | ||
| currentUrl.startsWith('data:') | ||
| ) { | ||
| if (!cancelled) setPreviewUrl(currentUrl); |
There was a problem hiding this comment.
🔥 The Roast: if (!cancelled) setPreviewUrl(currentUrl) — cancelled is initialized to false on line 38 and never mutated before this line. The cleanup function sets it to true, but that only runs when the effect is about to re-run or unmount, not during the initial synchronous pass. This check is about as useful as a screen door on a submarine.
🩹 The Fix:
| if (!cancelled) setPreviewUrl(currentUrl); | |
| setPreviewUrl(currentUrl); |
📏 Severity: nitpick
- Return actual base64 from binary file reads - Avoid rendering raw local asset paths before resolution - Store uploaded videos in emotion_videos - Delete local asset files on removal - Surface upload errors inline
Summary
Adds the ability to upload local images and videos for character avatars directly from the CharacterPanel UI, instead of requiring externally-hosted URLs.
Changes
How it works
Summary by Sourcery
Add support for uploading and storing local image/video assets for character avatars and wiring them into the CharacterPanel UI and storage layer.
New Features:
Enhancements:
Documentation: