Skip to content

Add MediaBrowser live_component with scope_folder_id - #495

Merged
ddon merged 6 commits into
BeamLabEU:devfrom
timujinne:dev
Apr 16, 2026
Merged

Add MediaBrowser live_component with scope_folder_id#495
ddon merged 6 commits into
BeamLabEU:devfrom
timujinne:dev

Conversation

@timujinne

@timujinnetimujinne commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extract PhoenixKitWeb.Live.Users.Media (1158 LOC LiveView) into reusable PhoenixKitWeb.Components.MediaBrowser live_component with optional scope_folder_id (hard-scoped embedding — variant A). /admin/media behavior preserved byte-for-byte when scope_folder_id: nil.
  • Upstream's ?view=all "All Files" feature (from Added image_set for correct file pulling on the front end image calls #490) ported into the new architecture. UUID-search and scope-safety preserved.
  • Merges upstream/dev with V97/V98 migrations, maintenance mode, OAuth health check, image_set component, and dev_docs PR reviews.
  • Plus two small admin UI improvements (see "Additional admin UI changes" below).

Changes

Storage (scope helpers)

  • within_scope?/2 predicate + scope-aware list_folder_tree/1, folder_breadcrumbs/2, list_folders/2, list_files_in_scope/2 (recursive CTE), count_orphaned_files/1
  • Mutator scope guards: create_folder, update_folder, delete_folder, move_file_to_folder, create_folder_link all return {:error, :out_of_scope} on violation
  • All new arities default to scope_folder_id \\ nil → backward-compatible

MediaBrowser live_component

  • 1313 LOC component + template. Parent LiveView media.ex shrunk to 75 LOC wrapper
  • Upload via progress: &handle_progress/3 callback (replaces Process.send_after polling incompatible with live_components). Reference pattern: media_selector_modal.ex
  • Controlled mode detected by presence of on_navigate attr. Parent-notify pattern: component emits {MediaBrowser, id, {:navigate, params}} → parent handle_infopush_patchhandle_paramssend_update(nav_params: ...) round-trip for ?folder/q/page/orphaned/view
  • scope_invalid detection (scope folder deleted) with UI banner; orphan filter UI hidden when scope set; upload target falls back to scope_folder_id at virtual root; out-of-scope navigation falls back to scope root with flash
  • ?view=all "All Files" feature ported: hides folders in grid, shows All Files ({count}) title, scope-safe via list_files_in_scope(scope, ...). UUID-search in list_files_in_scope preserved from upstream.

Runtime hotfixes discovered during browser smoke testing

  • phx-target={@myself} added to all 65 event bindings in template + function components (doesn't cascade from root div; each element with phx-click/change/submit/... needs its own target or events route to parent LV)
  • update/2 first-mount check via Map.has_key?(...) instead of not socket.assigns[...] (strict not raises on nil/list)
  • assign_new(:scope_folder_id, fn -> nil end) so templates using @scope_folder_id don't KeyError when parent omits attr
  • AssetsController extended to route phoenix_kit_consent.js to :phoenix_kit_legal app — previously returned 404 text/plain, breaking strict MIME check on any page using app_layout with Legal module enabled. Safe via try/rescue ArgumentError: parent apps without the Legal package see no impact.

Tests (52 new)

  • Storage scope contracts (scope_test.exs + media_browser_scope_test.exs)
  • LiveView integration (media_test.exs) — URL sync, deep links, auth, malformed page param
  • Component (media_browser_test.exs) — scope truncation, controlled/uncontrolled mode, orphan filter visibility, scope_invalid banner
  • URL-builder unit (media_url_test.exs) — 21 pure tests (no DB)
  • Extended ConnCase with sandbox + endpoint supervision for LiveView tests

Additional admin UI changes

Two small admin UI refactors are also included in this PR (committed to dev before the MediaBrowser work started):

  • ffda72f0 Remove globe language switcher from admin header — the standalone globe/locale dropdown is removed from the top admin bar; locale switching moves to the user avatar menu to reduce header clutter. Affects components/admin_nav.ex (-89 LOC), components/layout_wrapper.ex, modules/languages/README.md.
  • 1d09f49b Add toolbar_title and toolbar_actions slots to table_default — new slots on core/table_default.ex allow pages to inject a toolbar area above the table without extra wrapping. Migrated three admin pages to use the new slots: roles, activity index, integrations.

Both are non-breaking, additive/refactor changes. Called out explicitly for reviewer visibility.

Parent app impact

After merging to BeamLabEU:dev and pulling into parent apps:

  • Run mix phoenix_kit.update — installs phoenix_kit.js and adjusts root layout if needed.
  • Run mix ecto.migrate — applies V97 (per-item markup override) and V98 (image_set support) migrations coming from upstream.
  • No changes required in parent /admin/media usage — byte-for-byte preserved.
  • New embedding API: <.live_component module={PhoenixKitWeb.Components.MediaBrowser} id="..." scope_folder_id={folder_uuid} phoenix_kit_current_user={@current_user} on_navigate={:navigate}/> for scoped media browsers inside forms.

Out of scope (follow-up)

  • attr declarations at top of MediaBrowser
  • Inline <script>/<style> extraction from template
  • Bulk-mutator error aggregation for partial out-of-scope failures
  • Extract build_url/parse_page as public helpers to remove test duplication
  • Migrating MediaSelector / MediaSelectorModal onto MediaBrowser

Test plan

  • mix compile --warnings-as-errors — clean
  • mix format --check-formatted — clean
  • mix credo --strict — 0 issues on 504 files
  • mix dialyzer — clean (via pre-commit)
  • Unit tests: 21/21 pass locally (URL-builder)
  • Integration tests: compile correctly; pass in CI (local env has pre-existing psql client version mismatch)
  • Browser smoke on Decor3D Print (path: "/app") — Mode A: /admin/media behaves identically to pre-refactor (folder tree, file grid, upload, navigation, search, orphan filter, URL sync for ?folder/q/page/orphaned/view, ?view=all, malformed ?page=foo falls back to page 1). Mode B scoped data contracts verified via IEx.

- Drop language_switcher_dropdown from admin top bar in layout_wrapper.ex;
locale selection now lives in the user avatar dropdown only
- Remove dead admin_language_dropdown/1 and unused Phoenix.LiveView.JS alias
from admin_nav.ex
- Update Languages README: clarify that admin locale switches via user
menu, and the pre-login globe on the sign-in page stays for
unauthenticated visitors
- Extend table_default_with_cards with toolbar_title and toolbar_actions
slots, rendered in the same row as the view-toggle buttons. Toolbar row
is only shown when at least one of the three (title, actions, toggle)
is present. Flex-wrap layout keeps the row adaptive on mobile; the
view toggle stays desktop-only since mobile forces card view.
- Apply to roles page: move 'Create New Role' button from a centered
block above the table into toolbar_actions; add role count in
toolbar_title.
- Apply to activity page: move 'Clear filters' button from the filter
card into toolbar_actions; show total count in toolbar_title and
drop duplicate count from page subtitle.
- Apply to integrations page: move 'Add Integration' from the page
header actions into toolbar_actions; add connection count in
toolbar_title. Empty-state CTA already provides its own button.
Extract Users.Media LiveView into reusable PhoenixKitWeb.Components.MediaBrowser
accepting optional scope_folder_id for hard-scoped embedding (variant A).
/admin/media behavior preserved byte-for-byte (scope=nil).
Storage:
- Add within_scope?/2 predicate and scope-aware helpers:
list_folder_tree/1, folder_breadcrumbs/2, list_folders/2,
list_files_in_scope/2 (recursive CTE), count_orphaned_files/1
- Scope guards on all mutators (create_folder, update_folder, delete_folder,
move_file_to_folder, create_folder_link) return {:error, :out_of_scope}
Component:
- Parent LiveView shrunk to wrapper (1158 LOC to 38 LOC)
- Upload via progress: callback replaces Process.send_after polling
(handle_info absent, incompatible with live_components)
- Controlled mode detected by on_navigate attr presence: parent owns
navigation state and receives {MediaBrowser, id, {:navigate, params}}
notify messages; push_patch round-trip for ?folder/?q/?page/?orphaned
- scope_invalid detection with UI banner when scope folder deleted
- Upload target falls back to scope_folder_id at virtual root
- Orphan filter UI hidden when scope is set
- scoped_fallback? flash on URL-hack to out-of-scope folder
- Integer.parse fallback for malformed ?page param
- Guard update/2 first-mount check via Map.has_key? (not raises on nil)
- assign_new(:scope_folder_id) default so template @scope_folder_id
renders safely when parent omits the attr
Tests (52 total):
- 18 URL-builder unit tests (media_url_test.exs)
- 17 LiveView integration tests for URL sync and auth (media_test.exs)
- 17 component tests for scope behaviors (media_browser_test.exs)
- Storage scope contracts (scope_test.exs, media_browser_scope_test.exs)
- Extended ConnCase with sandbox + endpoint supervision
Out of scope (Task 7 follow-up):
- attr declarations on component
- Inline <script>/<style> extraction from template
- Bulk-mutator error aggregation for partial out-of-scope failures
- Extract build_url/parse_page as public helpers (test duplication)
Merge upstream/dev (commits a470bea..5f19ab8) which adds V97/V98
migrations, maintenance mode, OAuth health check, image_set component,
and a new ?view=all "All Files" feature in the media browser.
Conflicts in media.ex and media.html.heex resolved with our refactor
(MediaBrowser live_component), then upstream's ?view=all feature ported
into the new architecture:
- Add :file_view assign and navigate_view_all event to MediaBrowser
- Extend Storage.list_files_in_scope/2 with UUID-search via
`fragment("CAST(? AS TEXT) ILIKE ?", f.uuid, search)`
- URL round-trip for ?view=all (parent handle_params ↔ component
navigate/apply_nav_params); search and clear_search now preserve
file_view; toggle_orphan_filter explicitly resets view to nil
- Template: All Files sidebar button with active-state highlight,
All Files ({count}) section title, empty state, hide folder rows
and new-folder controls when file_view == "all"
- Preserves scope semantics: view=all under scope shows all files
within scope subtree via existing recursive CTE (no leakage)
Tests added: 2 URL-builder tests (view=all + q roundtrip, view=all
standalone), 2 UUID-search tests in scope_test.exs, 1 deep-link test
for /admin/media?view=all.
Refactor bonus: resolve_folder/2 and load_nav_files/7 extracted from
apply_nav_params to reduce complexity.
Runtime fixes discovered during browser smoke testing on Decor3D Print.
MediaBrowser event routing:
- Add phx-target={@Myself} to every phx-click/submit/change/keydown binding
in media_browser.html.heex (57 occurrences) and function components
folder_tree_node/move_folder_option (8 occurrences). phx-target does NOT
cascade from the root div; each event element needs its own target.
Without this, all clicks routed to the parent LiveView and crashed with
UndefinedFunctionError since the parent only defines mount/render.
- Add :myself attr to folder_tree_node and move_folder_option function
components so recursive calls propagate it correctly.
MediaBrowser KeyError on first mount:
- Replace `not socket.assigns[:uploaded_files]` with `not Map.has_key?(...)`
in update/2. Elixir's `not` is strict-boolean and raises ArgumentError
on nil or list values.
- Add assign_new(:scope_folder_id, fn -> nil end) so templates that use
@scope_folder_id don't crash with KeyError when parent omits the attr.
AssetsController — serve phoenix_kit_consent.js:
- Extend the valid-assets map to route by OTP app, not just filename.
phoenix_kit_consent.js lives in the phoenix_kit_legal package;
previously the controller returned 404 text/plain, breaking the strict
MIME check on any page that injects the consent banner script (that
then also broke other <script> tags and in effect killed LiveSocket
init on many pages).

@ddonddon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review — approve / merge

Large refactor, but architecturally clean. Full writeup in dev_docs/pull_requests/2026/495-media-browser-component/CLAUDE_REVIEW.md.

Verified:

  • Scope enforcement is completewithin_scope?/2 guards every mutator (create_folder, update_folder, delete_folder, move_file_to_folder, create_folder_link); recursive CTE in list_files_in_scope/2 uses Ecto param binding (no SQL injection); folder_breadcrumbs/2 truncates cleanly via drop_while.
  • All 62 server-event bindings have phx-target={@myself} — mechanically checked across media_browser.html.heex. Two apparent misses at :488 and :752 are false positives (forms carry submit + change + keydown under one target).
  • Lifecycle hotfixes correctMap.has_key?(socket.assigns, :uploaded_files) first-mount guard avoids the strict-not-on-nil trap; assign_new(:scope_folder_id, fn -> nil end) protects templates from KeyError when parent omits the attr.
  • AssetsController widening is safetry/rescue ArgumentError is the correct exception for Application.app_dir/2 when :phoenix_kit_legal isn't installed; parent apps without the package get a 404 (same as before), not a crash.
  • Backward compatibility preserved — all new scope_folder_id \\ nil defaults; /admin/media unscoped path unchanged.
  • Tests land where they matter — 52 new tests: scope CRUD guards (scope_test.exs, 519 LOC), component integration (media_browser_scope_test.exs, 233), URL sync + deep links (media_test.exs, 295), URL-builder units (media_url_test.exs, 167).
  • Two bundled admin UI changes are non-breaking — globe-switcher removal is pure deletion; table_default toolbar slots are additive, and the three migrated pages (roles/activity/integrations) are consistent.

Medium improvements (non-blocking):

  • Missing attr declarations on the live_component — author already listed as out-of-scope/follow-up, reasonable
  • folder_breadcrumbs/2 may N+1 on deep trees (only matters per-navigation, not per-file) — batch into a CTE if it becomes a hotspot
  • Verify the admin avatar dropdown exposes a language picker after the globe removal

Nitpicks:

  • Silent nil return from AssetsController when :phoenix_kit_legal isn't loaded — consider Logger.debug(...) to aid "why isn't my consent banner loading?" debugging

Nice work — the controlled/uncontrolled mode separation, scope_invalid banner, orphan-filter gating, and upload fallback to scope root are all well thought out. Ready to merge.

- delete_selected: check within_scope? for each file before calling
delete_file_completely. Previously scope was read but not applied,
allowing crafted WebSocket events to delete files outside scope.
- navigate_to_folder (uncontrolled mode): add within_scope? guard
before rendering folder contents. Previously a crafted navigate_folder
event with an out-of-scope UUID could expose sibling folder names.
Both are defense-in-depth fixes — admin-only, require crafted events,
but the scope enforcement contract should hold uniformly across all
code paths. Found during independent PR review by verifier agent.
@ddon

ddon commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Follow-up on 4a7057d5 — scope enforcement fixes

Re-reviewed. Two defense-in-depth holes closed correctly — nice catch, both were missed in my original review:

  • delete_selected (media_browser.ex:883-898) — now looks up each file via repo and guards Storage.within_scope?(file.folder_uuid, scope) before delete_file_completely. Closes the crafted-event deletion path.
  • navigate_to_folder uncontrolled branch (media_browser.ex:1056-1069) — now verifies the resolved folder is in scope and falls back to {nil, nil} (scope virtual root) on violation. Closes the crafted-event folder-name exposure.

Notes:

  • selected_folders was already safe via Storage.delete_folder(folder, scope) mutator-level guard, so no change needed there. ✅
  • Controlled mode of navigate_to_folder was never vulnerable — it round-trips through parent handle_paramsapply_nav_params/2resolve_folder/2, which already scope-checks.
  • Silent skip on out-of-scope delete is the right call for defense-in-depth, but the flash still reads "#{file_count + folder_count} item(s) deleted" using the selected count, not the actually deleted count. Minor, pre-existing — worth a small follow-up to report actual counts.

Verdict unchanged: approve / merge. Scope contract now holds uniformly across all code paths.

@ddon
ddon merged commit f9a4143 into BeamLabEU:devApr 16, 2026
ddon pushed a commit that referenced this pull request Apr 16, 2026
Covers scope enforcement across all mutators, event routing verification
(62 bindings), AssetsController widening for Legal app, and follow-up on
commit 4a7057d closing two defense-in-depth holes in delete_selected
and navigate_to_folder.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
timujinne added a commit to timujinne/phoenix_kit that referenced this pull request May 5, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timujinne@ddon