Uh oh!
There was an error while loading. Please reload this page.
Updated media browser component - #497
Merged
Merged
Conversation
Trash: soft-delete files to trash instead of permanent deletion. Trash sidebar button with count badge, restore/empty actions, auto-cleanup via PruneTrashJob after configurable retention period (default 30 days). V99 migration adds trashed_at column. Trashed files excluded from all file queries and orphan detection. Drag-drop upload: drop files from device directly onto folder content area. FolderDropUpload JS hook injects files into hidden LiveView upload input. Empty folders show "Drop files here" prompt. Breadcrumb padding: moved breadcrumbs and search bar inside the card body so they share the same padding as the grid/list content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix DateTime microseconds error by truncating to seconds for the trashed_at field. Clear folders list and hide new folder inputs when viewing trash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change delete confirmation modal from permanent deletion language to trash language since files now go to trash and can be restored. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Show "Trash is empty" with appropriate icon and hint instead of the generic upload prompt when viewing an empty trash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reset filter_trash and filter_orphaned in navigate_to_folder and navigate_view_all so switching views doesn't carry over stale state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reset filter_trash in apply_nav_params so controlled mode navigation (All Files, Root, folders) properly exits trash view without requiring a page refresh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass initial URL params to MediaBrowser on first mount so it loads the correct view immediately instead of defaulting to root and then correcting after WebSocket connects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove show_upload call from FolderDropUpload hook so files inject directly into the hidden upload input. Add inline progress bars in the card body. Fix upload input selector to search parent content area instead of the card-body scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ddon
requested changes
Apr 16, 2026
ddon
left a comment
Contributor
There was a problem hiding this comment.
Claude review — changes requested
Full writeup in dev_docs/pull_requests/2026/497-media-browser-improvements/CLAUDE_REVIEW.md. The trash/soft-delete feature is well-designed overall (V99 migration is idempotent, soft-delete respects scope, trashed files are excluded from normal listings, hydration fix is correct) but there are two tenant-isolation holes that need to close before merge.
Must fix
1. Trash view ignores scope_folder_id — CRITICALStorage.list_trashed_files/1 (storage.ex:1995-2004) and Storage.count_trashed_files/0 (storage.ex:2008-2012) take no scope parameter, and load_trashed_files/2 (media_browser.ex:1209-1213) doesn't pass one either. A scoped embed that toggles filter_trash will display every trashed file system-wide — cross-tenant leak.
Fix: add scope_folder_id arg to both Storage helpers and apply the same recursive-CTE scope subtree filter that list_files_in_scope/2 uses; thread scope through load_trashed_files/3.
2. Permanent-delete from trash skips scope check — CRITICALmedia_browser.ex:900-905:
ifsocket.assigns.filter_trashdoEnum.each(socket.assigns.selected_files,fnfile_uuid->Storage.delete_file_completely(file_uuid)# no within_scope? guardend)The non-trash branch (soft-delete) correctly repo.gets each file and checks within_scope?/2 before calling trash_file/1. The trash branch must mirror that pattern before delete_file_completely/1.
3. PruneTrashJob is never scheduled — HIGH
Worker is defined with moduledoc "Runs daily via cron", but no crontab entry in config/config.exs and the installer doesn't inject one into parent apps. As shipped, trash accumulates forever. Same aspirational-docstring pattern as Activity.PruneWorker, but since this PR introduces the worker, this is the right moment to wire it up — add {"0 3 * * *", PhoenixKit.Modules.Storage.Workers.PruneTrashJob} to the cron plugin and update lib/phoenix_kit/install/oban_config.ex so parent apps pick it up.
4. @type t on File schema missing trashed_at — HIGHschemas/file.ex:94-117 — one-line typespec update.
Should fix (not blocking)
- Schema docstring Status Flow (
schemas/file.ex:15-19) doesn't listtrashed Storage.list_files/1(storage.ex:1324-1331) doesn't filterstatus == "trashed"— low-risk since the admin UI goes throughlist_files_in_scope/2, but any external caller now sees trashed files- Flash count reflects selected items, not actually-deleted (already pre-existing; worth addressing with #1/#2)
Highlights (things done well)
- V99 migration idempotency + partial index on
trashed_at WHERE NOT NULL+ clean rollback ✅ - Soft-delete (non-trash branch of
delete_selected) already has scope guard ✅ list_files_in_scope/2+count_orphaned_files/1both addstatus != "trashed"filter ✅- Trash filter clears on URL navigation (fixes
75b1379a/2695ee39) ✅ - Hydration fix
01acc031usesMap.has_key?(assigns, :initial_params)— the correct guard (doesn't trip on nil) ✅ PruneTrashJobworker routes throughdelete_file_completely/1so variants + S3 + dedup tracking all get cleaned up ✅
ddon pushed a commit
that referenced
this pull request
Apr 16, 2026
Changes requested. Flags two critical scope-isolation holes in trash view (list_trashed_files/count_trashed_files ignore scope) and permanent delete from trash (no within_scope? guard), plus PruneTrashJob defined but never scheduled in any crontab. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. CRITICAL: Scope trash queries — list_trashed_files, count_trashed_files, and empty_trash now accept scope_folder_id and use recursive CTE to constrain results to the scope subtree. 2. CRITICAL: Permanent delete from trash now checks within_scope? before calling delete_file_completely, matching the soft-delete branch. 3. HIGH: Wire PruneTrashJob into Oban cron config in installer so parent apps pick up the daily 3 AM cleanup schedule. 4. HIGH: Add trashed_at to @type t spec and trashed to Status Flow docs. 5. Filter trashed files from Storage.list_files/1 for external callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Apr 16, 2026
…review restore_selected iterated client-trusted UUIDs from the selected_files MapSet and called Storage.restore_file/1 without a within_scope? check, mirroring the class of bug the PR #497 follow-up commit fixed for the permanent-delete branch. A scoped embed could push any UUID via toggle_select and restore files outside its scope. Apply the same repo.get + within_scope? guard the delete branch uses, and switch the flash to the actually-restored count since the guard may skip some selections. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Apr 16, 2026
…uard - Bump version 1.7.97 → 1.7.98 - CHANGELOG 1.7.98 entry covering PR #497 (V99 trash migration, PruneTrashJob daily cron, drag-drop upload via FolderDropUpload hook, URL-param first-mount hydration) and the restore_selected scope-guard fix Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexdont pushed a commit
to alexdont/phoenix_kit
that referenced
this pull request
Apr 19, 2026
Fix scope guard gap in restore_selected and append PR BeamLabEU#497 follow-up review Address PR review: make MediaBrowser uploads work in embedded components Parent LiveViews embedding MediaBrowser must register uploads on their own socket since LiveView routes upload channel events to the parent. Add setup_uploads/1 helper and handle_parent_info/2 catch-all. The component tracks its id with the parent via :register_component message, and uploads are routed back to the component via send_update with a :pending_upload key so folder placement uses the component's current state. - Hide All Files button when scoped (only on admin page) - Show scope folder name instead of "Root" in sidebar/header - Remove file count from header titles - Bypass scope check on initial upload placement (files start at root) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.