Add persona and global image galleries - #2361
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Bunny Review CompletedTip Review posted. The specimen has left the observation table.
|
🐰 Bunny ReviewBunny Merge Signal: Do Not MergeCaution DO NOT MERGE
Note Mode: 🧭 Specimen Summary
🔎 Isolated Defects
✅ Resolved Since Last Review
🧹 Nitpicks
🤖 Copy prompt for isolated Bunny findings✅ Control Checks
🧪 Observations
🧰 CI Status
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Review findings
Thread cleanup: resolved addressed Bunny threads |
Personas gain a Gallery tab mirroring the character gallery: a new persona-gallery collection with its own upload command, cascade-delete on persona removal, and expunge/profile-export/remote-runtime wiring. A new top-level Gallery panel provides a profile-wide image library (global-gallery) organized into optional flat folders (gallery-folders), with sorting, drag-an-image-onto-a-folder, and folder rename/delete that re-files images back to the root rather than deleting them. Management only; emoji/sticker tagging lands in a later change.
Addresses Bunny review on the global gallery: - upload_global_gallery_image now drops a folderId to root when the gallery-folders parent does not exist, so a stale UI race or remote caller cannot strand a row under a ghost folder (folder-delete cleanup only unfiles children of folders it actually deletes). - The lightbox folder move is now optimistic-with-rollback: a failed write reverts the dropdown and warns, instead of showing a folder the image was never filed into.
A failed move's rollback now only fires when its target is still the one on screen, so a stale failure in a rapid A→B→C sequence can't drag the lightbox back to an older folder. Addresses Bunny review follow-up.
The global and persona gallery batch uploads report failure when any file fails even though successful rows persist. Add a comment explaining this mirrors the character/chat gallery uploads on purpose, so the behavior isn't "fixed" in one gallery and split from the others.
- Validate global-gallery folderId in the generic create/update path (not just the upload command), so the lightbox move and remote callers can't file an image under a folder that doesn't exist. - Remove each gallery row's managed image file when its collection is expunged/cleared, so persona/global/media expunge and clear-all no longer orphan files in the shared gallery folder. - Serialize lightbox folder moves (disable the control while a write is pending) so a stale rollback can't restore an outdated folder value. - Trap Tab/Shift+Tab inside the global gallery lightbox, matching the persona/character lightboxes. - Batch uploads now report partial success honestly across all galleries (chat, character, persona, global) via a shared runner: successful rows are kept and surfaced, and only a total failure is reported as an error.
Snapshot the gallery file references, clear the rows first, then delete the files from the snapshot. Removing files before the row clear meant a failed clear could leave live rows pointing at deleted assets (broken references); clearing first leaves rows + files intact on failure, and only a post-clear file hiccup orphans files — the lesser evil. Addresses Bunny review follow-up.
runGalleryUploadBatch now returns the failed files with their reasons (and describeGalleryUploadFailures formats them), so a partial-failure toast reads "Some images didn't upload — kek3.png — too large" instead of a bare count. Gallery upload failures are deterministic (size/type/corrupt bytes), so this explains which file and why rather than offering a pointless retry — and preempts "image failed to upload for no reason" reports.
It's referenced only within PanelNavButtons now — the other consumer went away in the mobile UX rework this branch rebased onto — so drop the unused export to keep the lint clean.
c27ae28 to
696f0d9
Compare
The mobile panel grid (TOOLS_PANELS) predates the Gallery panel, so it was the only nav surface missing it after the mobile UX rework. Add the entry so Gallery is reachable on mobile, matching the desktop panel buttons.
| // caller could otherwise strand the row under a folder that was never created | ||
| // (or was just deleted) — and folder-delete cleanup only unfiles children of | ||
| // folders it actually deletes, leaving the orphan unreachable. Fall back to root. | ||
| let folder_value = match folder_id.map(str::trim) { |
There was a problem hiding this comment.
⚠️ MEDIUM: Global upload validates after creating the asset
Location: src-tauri/src/commands/storage/shared.rs:2958
A delightful inversion: the image bytes are persisted before the folder assignment is checked. If the
gallery-folderslookup fails here, the command returns an error while the managed file has already been born, and no cleanup branch can reach it. The generic create path validates first; this upload path conducts the experiment backward and leaks an orphan on validation-time storage failure.
Tip
Suggested fix: Validate the requested folder before persist_image_bytes, or wrap every post-persist error path in the same managed-file cleanup used for failed row creation.
| const qc = useQueryClient(); | ||
| return useMutation({ | ||
| mutationFn: ({ imageId, folderId }: { imageId: string; folderId: string | null }) => | ||
| storageApi.update("global-gallery", imageId, { folderId }), |
There was a problem hiding this comment.
⚠️ MEDIUM: Global moves still admit ghost folders
Location: src/features/catalog/gallery/hooks/use-global-gallery.ts:91
A charming relapse: uploads may pass through a backend folder check, but moving an existing global image writes through generic
storageApi.updatewith whateverfolderIdarrives. If that folder was deleted, stale, or fabricated before the mutation lands,global-gallery.folderIdcan still point at a parent row that no longer exists. The gallery inherits an orphaned filing system and then politely renders the lie.
Tip
Suggested fix: Route moves through a focused backend command that validates the destination folder, or add the same non-null folderId existence check to generic global-gallery update and patch normalization.
There was a problem hiding this comment.
This one's a false positive — the move path already enforces the folder-existence check this asks for.
Trace:
useMoveGlobalGalleryImage→storageApi.update("global-gallery", id, { folderId })(use-global-gallery.ts)- →
invokeTauri("storage_update", …)→storage_update→storage_update_inner(entities.rs) storage_update_innercallsvalidate_gallery_folder_for_patch(state, &entity, &patch)?— the same family of per-entity folder validators the generic create path uses. When the patch touchesfolderIdit runsvalidate_gallery_folder_assignment, which errors withgallery-folders/{id} was not foundif the destination folder doesn't exist.
So a move to a deleted / stale / fabricated folder is rejected server-side; the ghost folderId never reaches storage. Both move entry points — the lightbox folder <select> and drag-to-folder-chip — go through useMoveGlobalGalleryImage, so they're both covered. "Generic storageApi.update" isn't unvalidated: storage_update_inner fans out to per-entity folder guards (chat / connection / lorebook / gallery), and the gallery one was added precisely to address the earlier folder-reference finding.
What's genuinely left is the check→write TOCTOU, which for a local single-process store is effectively nil, and folder-delete already unfiles its own children.
Flagging for maintainer visibility (@Xelvanis) — happy to route moves through a dedicated, narrower backend command if you'd prefer that over the generic update path, but the existence check is enforced today.
upload_global_gallery_image persisted the managed image file and only then looked up the destination folder. A storage error during that lookup would bubble up after the file was already written, leaking an orphan the row-create cleanup path never sees. Resolve the folder first so any lookup failure aborts before persisting.
| // actually deletes, leaving the orphan unreachable; fall back to root. Doing the | ||
| // lookup first also means a storage error here can't leak an already-persisted | ||
| // managed file (only the row-create path below has file cleanup). | ||
| let folder_value = match folder_id.map(str::trim) { |
There was a problem hiding this comment.
⚠️ MEDIUM: Folder validation is still only a pre-write rumor
Location: src-tauri/src/commands/storage/shared.rs:2953
The command observes
gallery-foldersbefore persisting bytes, then later creates aglobal-galleryrow carrying that same id. If the folder is deleted between the lookup andcreate_immediate, the row can still be born with a non-nullfolderIdpointing at a folder the delete cleanup never saw. Contract 1's leak is improved; Contract 2's referential invariant remains incompletely sedated.
Tip
Suggested fix: Make the folder check and row creation one coherent storage-side operation, or revalidate the non-null folder id immediately before commit and clean the just-persisted asset on validation failure.
3 participants
Linked issue
Closes #2360 (partially).
This is the first PR in a planned series implementing that feature request. It delivers the gallery foundations only (persona galleries + a global gallery panel) — the management surface the rest of the feature builds on. Emoji/sticker tagging, the Conversation-mode selectors, and AI reactions follow in later PRs, so this PR intentionally does not close the request.
Why this change
What changed
persona-gallerystorage collection +persona_gallery_uploadcommand, with cascade-delete when a persona is removed, plus expunge, profile-export, and remote-runtime wiring.global-gallerycollection plus a flatgallery-folderscollection. Supports sorting (newest/oldest/name asc/desc), create/rename/delete folders (deleting a folder re-files its images to the root rather than deleting them), uploading into a folder, and moving images by dragging onto a folder chip or via a lightbox picker.src-tauri/src/lib.rs, the HTTP dispatcher, and the remote-runtime allowlist.Refactor impact
Primary owner: catalog (galleries) and Rust storage.
Impact areas reviewed:
contracts.rscollections + cleanup enum,entities.rscleanup wiring,shared.rsupload helper,media.rscommands,admin.rsexpunge scopes)storage-api.tsinvalidation,image-generation-api.tsupload surface)RightPanel.tsx,PanelNavButtons.tsx) — added the Gallery panel entrystorage.tsentity-name union)Boundary notes:
src/engine/capabilities/storage.ts) gained the new collection names.uploadPersona/uploadGlobalsurfaces.catalog/galleryandcatalog/personas.DeletePersonaGallery/ClearGalleryFoldercleanups, and a global-gallery upload helper.persona_gallery_uploadandglobal_gallery_uploadto the allowlist + HTTP dispatch.Pressure points touched:
src-tauri/src/lib.rscommand registration — added the two upload commands. ModeSurface, GameSurface, shared mode UI, and import modules are untouched; nothing touches the RP/Game/Conversation runtime.Validation
pnpm typecheck,pnpm build,pnpm check:architecture,pnpm check:docs, or fullpnpm checkwhen warranted)pnpm checkpasses before PR push/handoffManual verification notes
Ran individually (the local
check:line-endingsstep trips on pre-existing CRLF in unrelated files, so the chainedpnpm checkbails early; the committed versions of those files are LF, so CI is unaffected):pnpm check:frontend(tsc -b) — passespnpm check:rust(cargo check --workspace) — passespnpm check:architecture— passes (no dependency violations)pnpm check:unused(knip) — cleanpnpm check:discovery— passesManual Tauri verification: uploaded images to the global gallery, created/selected/renamed/deleted folders, sorted, and moved images both by dragging onto a folder chip and via the lightbox picker; verified the persona Gallery tab uploads and displays. Confirmed in both dark and light themes.
Feature Discoverability
Check exactly one:
src/features/shell/discovery/because this PR adds or materially changes a user-discoverable feature, workflow, setting, mode, panel, import path, agent, media capability, or advanced tool.Reason:
src/features/shell/discovery/discovery-entries.jsongained aglobal-galleryentry and the panel-target allowlists were extended.Docs and release impact
README.mdCONTRIBUTING.mddocs/developer/AGENTS.mdUI evidence