fix: stage character asset mutations#6
Conversation
Reviewer's GuideStages character asset lifecycle in the chat character panel so that uploads/deletes are applied transactionally on panel save, rolled back on cancel/back, and protected against upload race conditions, while updating asset path parsing for timestamped filenames. Sequence diagram for staged character asset lifecycle in CharacterPanelsequenceDiagram
actor User
participant CharacterEditor
participant CharacterPanel
participant AssetCleanup
User->>CharacterEditor: handleEmotionAssetUpload / handleBaseImageUpload
CharacterEditor->>CharacterEditor: markUploadedAsset
CharacterEditor->>CharacterEditor: markRemovedAsset
User->>CharacterEditor: handleSave
CharacterEditor->>CharacterEditor: collectCharacterLocalAssetPaths
CharacterEditor->>CharacterEditor: diffLocalAssetPaths
CharacterEditor->>CharacterPanel: onSave(updatedCharacter, lifecycle)
CharacterPanel->>CharacterPanel: setSessionCreatedAssets
CharacterPanel->>CharacterPanel: setPendingDeleteAssets
alt Panel Save
User->>CharacterPanel: handleSave
CharacterPanel->>CharacterPanel: collectCollectionLocalAssetPaths
CharacterPanel->>AssetCleanup: deleteAssetPaths(stagedDeletes)
else Panel Cancel/Back/Close
User->>CharacterPanel: handleCancel
CharacterPanel->>CharacterPanel: collectCollectionLocalAssetPaths
CharacterPanel->>AssetCleanup: deleteAssetPaths(createdOnlyInSession)
end
Sequence diagram for guarded character asset uploads in ImageUploadersequenceDiagram
actor User
participant ImageUploader
participant uploadCharacterAsset
participant deleteCharacterAsset
User->>ImageUploader: handleFile(file)
ImageUploader->>ImageUploader: uploadSequenceRef += 1
ImageUploader->>uploadCharacterAsset: uploadCharacterAsset(characterId, emotion, file, type)
uploadCharacterAsset-->>ImageUploader: path
ImageUploader->>ImageUploader: isCurrentUpload?
alt Stale upload
ImageUploader->>deleteCharacterAsset: deleteCharacterAsset(path)
else Current upload
ImageUploader->>ImageUploader: getCharacterAssetUrl(path)
ImageUploader->>ImageUploader: setPreviewUrl / setUploading(false)
ImageUploader->>User: onUpload(path, type)
end
User->>ImageUploader: handleRemove
ImageUploader->>ImageUploader: uploadSequenceRef += 1
ImageUploader->>ImageUploader: setPreviewUrl(null), setUploading(false)
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 an asset lifecycle management system to track and clean up character assets during creation and editing. It introduces state tracking for session-created and pending-delete assets, ensuring unreferenced files are removed on save or cancel. Additionally, the ImageUploader was updated to handle race conditions during concurrent uploads. A review comment suggests optimizing the deletion logic in CharacterPanel.tsx by using direct set iterations instead of array spreading to improve efficiency.
| const stagedDeletes = new Set( | ||
| [...pendingDeleteAssets].filter((path) => !referencedPaths.has(path)), | ||
| ); | ||
| for (const path of sessionCreatedAssets) { | ||
| if (!referencedPaths.has(path)) stagedDeletes.add(path); | ||
| } |
There was a problem hiding this comment.
This logic can be simplified and made more efficient by avoiding intermediate array spreading and filtering. Using direct loops over the sets is cleaner and avoids unnecessary allocations.
| const stagedDeletes = new Set( | |
| [...pendingDeleteAssets].filter((path) => !referencedPaths.has(path)), | |
| ); | |
| for (const path of sessionCreatedAssets) { | |
| if (!referencedPaths.has(path)) stagedDeletes.add(path); | |
| } | |
| const stagedDeletes = new Set<string>(); | |
| for (const path of pendingDeleteAssets) { | |
| if (!referencedPaths.has(path)) stagedDeletes.add(path); | |
| } | |
| for (const path of sessionCreatedAssets) { | |
| if (!referencedPaths.has(path)) stagedDeletes.add(path); | |
| } |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the stage character editor’s asset-handling flow so uploaded/replaced/removed character assets are treated as session-scoped until the user commits changes, and also adds race protection to prevent stale uploads from “winning” after rapid user interactions.
Changes:
- Add upload sequence tokens + stale-upload cleanup in
ImageUploaderto guard against concurrent/abandoned uploads. - Track session-created assets and deferred deletions in
CharacterPanel/CharacterEditor, cleaning up only on panel Save (and only session-created assets on Cancel/Back). - Broaden local asset path parsing to recognize the versioned upload filename format.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/webuiapps/src/lib/characterAssetUpload.ts | Updates local character asset path parsing/validation to accept versioned upload filenames. |
| apps/webuiapps/src/components/ChatPanel/ImageUploader.tsx | Adds upload sequencing and stale-upload cleanup; disables interactions while uploading. |
| apps/webuiapps/src/components/ChatPanel/CharacterPanel.tsx | Introduces session asset lifecycle tracking and defers local asset cleanup until commit/cancel semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const path of new Set(paths)) { | ||
| try { | ||
| await deleteCharacterAsset(path); | ||
| } catch (err) { | ||
| console.warn('Failed to delete character asset:', err); | ||
| } | ||
| } |
|
|
||
| const match = path.match( | ||
| /^\/characters\/([A-Za-z0-9_-]+)\/emotions\/([A-Za-z0-9_-]+)\.([A-Za-z0-9]+)$/, | ||
| /^\/characters\/([A-Za-z0-9_-]+)\/emotions\/([A-Za-z0-9_-]+)(?:-\d{13}-[a-z0-9]+)?\.([A-Za-z0-9]+)$/, |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| onUpload(path, type); | ||
| } catch (err) { | ||
| if (!isCurrentUpload()) return; |
There was a problem hiding this comment.
🔥 The Roast: Your stale-upload guard is so focused on the happy path it completely ghosted the error path. When a stale upload throws during getCharacterAssetUrl, you bail early like a bad Tinder date — but you leave the uploaded file on the server like an unwanted houseplant. I've written this exact bug, and I'm still ashamed.
🩹 The Fix: Lift path out of the try block so it's accessible in catch, then clean up the stale asset before returning:
let path: string | undefined;
try {
...
path = await uploadCharacterAsset(characterId, emotion, file, type);
...
} catch (err) {
if (!isCurrentUpload()) {
if (path) await cleanupStaleUpload(path);
return;
}
...
}📏 Severity: warning
|
|
||
| const markUploadedAsset = (url: string) => { | ||
| if (!isLocalCharacterAssetPath(url)) return; | ||
| setCreatedAssets((current) => new Set(current).add(url)); |
There was a problem hiding this comment.
🔥 The Roast: new Set(current).add(url) is the React equivalent of buying a new house because you got a new throw pillow. If url is already in the set, you're forcing a re-render for absolutely nothing. Same thing happens in markRemovedAsset a few lines down — you're running a two-for-one special on unnecessary allocations.
🩹 The Fix:
| setCreatedAssets((current) => new Set(current).add(url)); | |
| setCreatedAssets((current) => (current.has(url) ? current : new Set(current).add(url))); |
📏 Severity: nitpick
Code Review Roast 🔥Verdict: 2 Issues Found (2 new, 0 carried forward) | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The asset lifecycle logic in 💀 Worst part: Two lines of code apparently decided indentation is optional. One in 📊 Overall: Like a mostly-clean apartment with two socks left on the floor. Functional, but the small messes nag at you. Files Reviewed (4 files)
Previously reviewed issues:
Fix these issues in Kilo Cloud Reviewed by kimi-for-coding · full review after force push Reviewed by kimi-for-coding · 3,697,398 tokens |
SweetSophia
force-pushed
the
fix/avatar-media-classification
branch
from
17ae17f to
02957d2
Compare
SweetSophia
force-pushed
the
fix/avatar-asset-lifecycle
branch
from
90f06b7 to
24fb6ac
Compare
SweetSophia
force-pushed
the
fix/avatar-media-classification
branch
from
02957d2 to
716e71e
Compare
SweetSophia
force-pushed
the
fix/avatar-asset-lifecycle
branch
from
24fb6ac to
8d89720
Compare
SweetSophia
force-pushed
the
fix/avatar-media-classification
branch
2 times, most recently
from
b4f7e39 to
c7c280a
Compare
| let url: string | null | undefined; | ||
| try { | ||
| url = await getCharacterAssetUrl(path); | ||
| } catch { |
There was a problem hiding this comment.
🔥 The Roast: Your catch block is so empty it echoes. getCharacterAssetUrl could be screaming about a network timeout, a missing file, or a permissions issue, but we'll never know because you caught the error and immediately sent it to /dev/null. It's like taking your car to a mechanic who says "something's wrong" and hands you a bill.
🩹 The Fix: Add an error parameter to the catch and preserve it — either log it with console.warn before cleaning up, or pass it along as cause in the thrown error: throw new Error('Failed to retrieve asset URL', { cause: err }). Debugging production issues is hard enough without you playing hide-and-seek with the stack trace.
📏 Severity: suggestion
SweetSophia
force-pushed
the
fix/avatar-asset-lifecycle
branch
from
4a3bd53 to
8cb4592
Compare
SweetSophia
force-pushed
the
fix/avatar-asset-lifecycle
branch
from
8cb4592 to
167b00c
Compare
|
|
||
| if (expectedUrlRef.current === path || !expectedUrlRef.current) { | ||
| const isVidLocal = isVid; | ||
| const isVidLocal = isVid; |
There was a problem hiding this comment.
🔥 The Roast: Your indentation went on vacation and forgot to come back. This line is so far left it might as well be running for office.
🩹 The Fix:
| const isVidLocal = isVid; | |
| const isVidLocal = isVid; |
📏 Severity: nitpick
| }); | ||
|
|
||
| it('detects image asset URLs with query strings and hashes', () => { | ||
| it('detects image asset URLs with query strings and hashes', () => { |
There was a problem hiding this comment.
🔥 The Roast: This it block is so unindented it looks like it's trying to escape the describe. Did your spacebar break halfway through writing this test?
🩹 The Fix:
| it('detects image asset URLs with query strings and hashes', () => { | |
| it('detects image asset URLs with query strings and hashes', () => { |
📏 Severity: nitpick
Summary
Validation
Summary by Sourcery
Stage character asset lifecycle so editor changes only delete local assets on save and roll back session-only uploads on cancel or back.
Bug Fixes:
Enhancements: