Uh oh!
There was an error while loading. Please reload this page.
Add V108 + drag-and-drop core + PR #506 review follow-up - #512
Merged
Conversation
Three independent admin lists previously sat in insertion order (latest first or alphabetical) with no user-driven order: - `phoenix_kit_entities` — the entity-definitions list at `/admin/entities` - `phoenix_kit_cat_catalogues` — the catalogues index at `/admin/catalogue` - `phoenix_kit_cat_items` — items shown on a catalogue's detail page and inside categories V108 adds a nullable `position integer` to each with a default of `0` so legacy rows can keep insertion-order shape until somebody drags; the LV reorder handlers re-index the visible group to `1..N` on the first user drag, so the default is only ever observed transiently. Idempotent — `ADD COLUMN IF NOT EXISTS` matches the project pattern from V107 and earlier; safe to re-run mid-migration. Down/1 reverses column-by-column AND updates the `COMMENT ON TABLE phoenix_kit IS` marker from `'108'` back to `'107'` (the source of truth read by `migrated_version/1`). Categories and smart-catalogue rules already carry their own `position` columns (V87 and V102 respectively), so they're untouched here. The `phoenix_kit_entity_data` table already got both `position` and the `(entity_uuid, position)` composite index in V81. No indexes on the three new columns themselves — entity / catalogue / item-per-catalogue lists are small (≤ a few hundred rows) and have other indexed scope filters; a position-only btree would burn write amplification for no real read win.
…e, :draggable opt-out
Three additive changes to the core DnD primitives so consumer modules
(phoenix_kit_entities, phoenix_kit_catalogue) can wire reorder UIs
without forking the hook or the components.
## SortableGrid hook (`priv/static/assets/phoenix_kit.js`)
- Reads `data-sortable-group` and passes it to SortableJS as the
`group` config. Tables sharing a group name can exchange items via
cross-container drag (catalogue's "items move between categories"
use case). When unset, behavior is unchanged — single-container
reorder only.
- On `onEnd`, detects cross-container drops via `evt.from !== evt.to`.
Cross-container payloads carry `moved_id` (the dragged item's
data-id) and the source container's scope attrs prefixed with
`from*` alongside the destination's regular scope attrs. The push
routes through the destination's `data-sortable-event` so the LV
handler is co-located with the table the item ended up in.
- Pulls a `readScope/1` helper that translates `data-sortable-scope-*`
attrs (camelCase via DOMStringMap) into a `{key: value}` map. The
prefix-length check (`key.length > "sortableScope".length`) ensures
a bare `data-sortable-scope` attr can't masquerade as scope data.
- The `onEnd` body is now wrapped in try/catch — a single bad dataset
value (corrupt scope attr, missing source container after a fast
unmount) logs to `console.error` instead of leaving SortableJS in a
half-initialized state with the LV unable to reorder again.
- A leading docstring spells out the "trust your own DOM" assumption
for the `.sortable-item[data-id]` query — if a third-party script
injects sortable-item nodes, IDs may be poisoned, so the LV handler
should reject unknown IDs at the server side.
## TableCardView hook (`priv/static/assets/phoenix_kit.js`)
Added `updated()` callback that re-applies the user's saved view mode
after every LV-driven render. Without it, morphdom resets the
runtime `md:hidden` class toggles back to template defaults on each
diff (e.g. after a SortableJS drop fires a reorder event, the LV
re-renders and the card view snaps back to the table view). The hook
now caches `currentMode` on mount + every toggle/event and replays it
in `updated()` so the chosen mode survives.
## `<.draggable_list>` (`components/core/draggable_list.ex`)
New `:draggable` boolean attr (default `true`). When false, the
container renders without the `phx-hook="SortableGrid"` and items
skip the `cursor-grab` styling — useful when the list is too short
to reorder (length ≤ 1) so the affordance doesn't lie. Default
`true` keeps every existing caller unchanged.
## `<.table_default>` (`components/core/table_default.ex`)
Four new attrs — `:on_reorder`, `:reorder_scope`, `:reorder_group`,
`:item_id` — wire the card-view container as a SortableGrid hook
target when set. Each rendered card gets `class="sortable-item
cursor-grab active:cursor-grabbing"` + `data-id={item_id_fn.(item)}`
so a user toggled into card view (mobile or desktop click) gets the
same DnD experience as the table view. The `:item_id` defaults to
`& Map.get(&1, :uuid)` so the conventional schema shape works with
zero config; any non-uuid keying passes a custom function.
A footer row joins the card-actions slot when sortable: drag-handle
icon (`hero-bars-3` with cursor-grab styling, `gettext("Drag to
reorder")` tooltip) bottom-left, action buttons bottom-right.
`use Gettext, backend: PhoenixKitWeb.Gettext` added to match the
convention in `flash.ex` / `input.ex` / `integration_picker.ex`.
A private `build_sortable_scope_attrs/1` helper translates a
`%{key => value}` scope map into `[{"data-sortable-scope-key" =>
value}, ...]` tuples, spread via `{@reorder_scope_attrs}` in HEEX
(Phoenix's attribute-spread escape applies, so untrusted-input
callers don't introduce an XSS path).
`mix format` clean. `mix credo --strict` clean (7564 mods/funs, 0
issues).C12 triage on the V108 / DnD core work surfaced that there are no tests under `test/phoenix_kit_web/components/core/` — the directory doesn't exist. Several components there have grown to non-trivial attr surfaces that warrant rendered-HTML coverage, but landing a single fixture in an empty test dir alongside a feature PR muddles the diff. Recording the gap as a TODOs section so a future component-coverage sweep has the inventory ready (`<.draggable_list>` :draggable branches, `<.table_default>` :on_reorder/:reorder_scope/ :reorder_group/:item_id branches, the form primitives, plus `<.flash>` / `<.modal>` if their complexity has grown).
PR BeamLabEU#506 (Support arity-2 dynamic_children callback with locale, merged 2026-04-24) was APPROVED with two NITPICKs. Both closed: ## Test coverage was half-tautological The previous `"invoke_dynamic_children/3 dispatch"` describe block defined two anonymous functions, invoked them with `.(%{})` and `.(%{}, "en-US")`, and asserted the counters incremented. As Claude's review pointed out, that tested Elixir's function-call semantics rather than the sidebar's actual dispatch logic. Added a `@doc false` test-only delegate `AdminSidebar.__invoke_dynamic_children_for_test__/3` that calls the private `invoke_dynamic_children/3`, then rewrote the describe block with four assertion-pinned tests: - arity-1 callback receives only the scope - arity-2 callback receives both scope and locale - arity-2 callback handles a nil locale gracefully - return value is propagated unchanged The delegate carries an explicit `@doc false` so it's not part of the runtime surface — it exists solely to let the unit suite reach the private dispatcher without coupling to LV rendering. ## @TypeDoc on dynamic_children_fn The inline `#` comment above `@type dynamic_children_fn` wasn't picked up by ExDoc / `h Tab`. Replaced with a `@typedoc` block that documents both arities, the explicit-locale rationale, and the `nil` semantic. `mix test` for the test file: 6 tests, 0 failures (2 original + 4 new). `mix format`, `mix credo --strict` clean, `mix compile --warnings-as-errors` clean. PR-folder FOLLOW_UP.md filed under `dev_docs/pull_requests/2026/506-dynamic-children-locale/`.
Merged
5 tasks
ddon pushed a commit
to BeamLabEU/phoenix_kit_catalogue
that referenced
this pull request
May 2, 2026
C12 triage on `b451d64`'s DnD work surfaced four real issues; this
batch closes them. C12.5 deep-dive categories all clean (most N/A
in this diff: no PubSub/translations/auth additions; existing
Gettext wrapping verified; activity-log helpers re-used).
## Cross-container atomicity (BUG-MEDIUM)
`CatalogueDetailLive`'s cross-category drop did
`Catalogue.move_item_to_category/3` and `Catalogue.reorder_items/4`
as separate context calls. If the move committed but the reorder
rolled back (transaction error, scope mismatch, future cap-exceeded),
the item ended up in the new category with stale position values —
inconsistent with what the user just dragged.
Closes by adding `Catalogue.move_item_and_reorder_destination/4`
which wraps both context fns in a single `repo().transaction/1`.
Either both land or both roll back; the LV's `with` chain calls
this single fn instead of chaining the two. Activity-log fan-out
for `item.moved` and `item.reordered` still happens through their
normal paths so the audit trail keeps full attribution.
## Length cap @ 1000 + dedup (IMPROVEMENT-MEDIUM)
Parity with the entities reorder paths. Each of
`Catalogue.reorder_catalogues/2`, `reorder_categories/4`,
`reorder_items/4`, and `Rules.reorder_catalogue_rules/3` now:
- Returns `{:error, :too_many_uuids}` when the input list exceeds
the shared `@reorder_max_uuids 1000` cap. Protects the
transaction from N+1 unbounded write storms; even a workspace
with hundreds of catalogues / categories / items per group
never paints a thousand at once.
- Dedups inputs (last-occurrence wins via `dedupe_keep_last/1`).
A double-drop or stale DOM no longer issues redundant per-uuid
writes inside the transaction.
## Audit-trail gaps on rejection + DB error (BUG-HIGH)
The reorder paths logged only on `:ok`. Now they log on every
branch via two new private helpers, `log_reorder_rejected/5` and
`log_reorder_db_error/5`:
- Early rejection (`:too_many_uuids`, `:not_siblings`,
`:wrong_scope`) lands an audit row with `metadata.db_pending:
true` and `metadata.rejected: <atom>`. The user-initiated action
is still attributable even though no DB write happens.
- DB transaction failure on success-shape input lands a row with
`metadata.db_pending: true` (no `rejected` flag — the input was
valid, the transaction just failed). Lets audit consumers tell
rejected and failed apart.
`Rules.reorder_catalogue_rules/3` mirrors the shape with its own
`log_smart_rules_reorder_rejected/4` and
`log_smart_rules_reorder_db_error/3` (smart-rule activity rows go
to the `item` resource_type, not a generic shared helper).
## Cleanup (credo)
- Refactored `reorder_catalogues/2` to extract a
`write_catalogue_positions/1` private helper. Drops the credo
"Function body too deep" finding.
- The pre-existing `cond` with one non-`true` branch in
`CatalogueDetailLive.handle_event("reorder_items", _)` (a credo
refactoring opportunity) becomes a clean `if` / `else` plus a
new private `apply_in_scope_item_reorder/4` helper.
## Verification
- `mix format --check-formatted` clean
- `mix credo --strict` clean (1220 mods/funs, 0 issues)
- `mix compile` clean apart from the documented standalone-vs-parent
warnings (the `<.table_default>` `:on_reorder` / `:reorder_scope` /
`:reorder_group` / `:item_id` attrs and `<.draggable_list>`
`:draggable` attr land via the in-flight `BeamLabEU/phoenix_kit#512`
DnD core PR; until that merges + a release cuts, standalone
catalogue tests will continue to fail per
`feedback_run_tests_via_parent.md`)ddon pushed a commit
that referenced
this pull request
May 2, 2026
PR #512 review verdict: APPROVE. Six findings, all NITPICK / LOW. Two are safe to close inline: ## reorder_scope camelCase round-trip — documented Elixir map key `:category_uuid` round-trips through `build_sortable_scope_attrs/1` (lowercase + dash) and the `SortableGrid` hook's `readScope/1` (strip "sortableScope" prefix + lowercase first char) as `"categoryUuid"` in the LV handler payload. The original `:reorder_scope` doc didn't mention this; consumers following Phoenix params instinct would write `%{"category_uuid" => uuid}` and silently miss the param. Added the round-trip example to the attr `:doc`. ## draggable_list data-id always emitted The PR introduced `data-id={if @Draggable, do: @item_id_fn.(item)}` which strips `data-id` on the non-DnD path. `data-id` is useful even without DnD — click-to-select handlers, integration test selectors, JS that reads source-of-truth IDs off the element. Restored the unconditional emission and updated the `:draggable` attr doc to state the guarantee explicitly. ## on_reorder asymmetry — clarified The doc already noted that `:on_reorder` only wires the card view and the consumer owns the table view's tbody. Tightened the wording to make the desktop/mobile asymmetry impossible to miss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
May 4, 2026
PR #514 extracted Customer Service to phoenix_kit_customer_support and renamed the module surface; reviewed and closed two minor findings. Changes (review close-outs): - Backfill V108 docstring section in lib/phoenix_kit/migrations/postgres.ex (was missing — pre-existing gap from PR #512 that this PR was the natural place to close) - Drop unused _prefix arg from rename_role_permission/4 in v109.ex (table name is already prefix-qualified at the call site) Skipped on purpose: rewriting V109's DO \$\$ blocks as parameterized queries. V109 is unpublished but the values are migration-time constants, the existing implementation is tested and idempotent, and minimizing surface area before publish beats stylistic polish. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mdon added a commit
to mdon/phoenix_kit
that referenced
this pull request
May 5, 2026
Triage of CLAUDE_REVIEW.md against current code: all six findings (5 NITPICKs + 1 LOW) addressed pre-existing — `:reorder_scope` camelCase round-trip doc, `:on_reorder` asymmetry doc, `data-id` unconditionally emitted, V108 sort stability covered by downstream catalogue's `[asc: :position, asc: :name]` ordering. FOLLOW_UP.md records the audit.
5 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
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.
Summary
Four commits on top of `upstream/dev` (`d3b06b13`):
position integer DEFAULT 0on three listsurfaces (
phoenix_kit_entities,phoenix_kit_cat_catalogues,phoenix_kit_cat_items) so admin lists can persist user-drivenorder. Pure schema add; no backfill, no indexes (lists are small).
<.draggable_list>gaineda
:draggableboolean attr (defaulttrue) so callers candisable DnD without duplicating card markup.
<.table_default>gained four sortable attrs (
:on_reorder,:reorder_scope,:reorder_group,:item_id) that wire the card-view container asa
SortableGridhook target.phoenix_kit.jsgainedcross-container drop detection + scope passing (via
data-sortable-scope-*attrs anddata-sortable-group) plus anupdated()callback onTableCardViewso the user's chosenview-mode survives LV re-renders. Consumers in this batch:
phoenix_kit_entities(records DnD) and the in-flightphoenix_kit_cataloguework (catalogues / categories / items DnD,including cross-category item moves).
coverage gap C12 triage flagged: there are no tests under
test/phoenix_kit_web/components/core/. Inventoried for a futurecoverage sweep (
<.draggable_list>,<.table_default>, the formprimitives, possibly
<.flash>/<.modal>).Claude's review of the merged arity-2
dynamic_childrenwork.PR follow-up(s)
PR Support arity-2 dynamic_children callback with locale #506 — `3d4bf2f1` "PR Support arity-2 dynamic_children callback with locale #506 review follow-up — close the
two NITPICKs". The merged arity-2
dynamic_childrenworkshipped with a half-tautological dispatch test (anonymous fn
invoked via
.(), asserting Elixir's call semantics rather thanthe sidebar's actual dispatcher) and an inline
#comment ondynamic_children_fnthat ExDoc didn't pick up.Fix: added a `@doc false` test-only delegate
`AdminSidebar.invoke_dynamic_children_for_test/3` that
calls the private `invoke_dynamic_children/3`. Rewrote the
describe block with four assertion-pinned tests covering arity-1,
arity-2, nil locale, and return-value propagation. Replaced the
inline comment on `dynamic_children_fn` with a proper
`@typedoc`. FOLLOW_UP.md filed under
`dev_docs/pull_requests/2026/506-dynamic-children-locale/`.
Quality sweep
The big work, structured by commit.
V108 migration (`38b696cd`)
`lib/phoenix_kit/migrations/postgres/v108.ex` adds nullable
`position integer` (default 0) to three tables — the
entity-definitions list, the catalogues index, and the catalogue
items list. Idempotent (`ADD COLUMN IF NOT EXISTS` matches the
project pattern from V107 and earlier). `down/1` drops the
columns and reverts the `COMMENT ON TABLE phoenix_kit IS` marker
from `'108'` back to `'107'`.
No indexes on the new `position` columns. The lists are small
(≤ a few hundred rows in practice) and have other indexed scope
filters; a position-only btree would burn write amplification for
no real read win. `phoenix_kit_entity_data` already got both
`position` and the `(entity_uuid, position)` composite index
in V81, so it's untouched here.
`@current_version` bumped from 107 → 108 in
`lib/phoenix_kit/migrations/postgres.ex`.
DnD core infra (`b27e647e`)
Three additive changes to the core DnD primitives so consumer
modules wire reorder UIs without forking the hook or the components.
`SortableGrid` hook (`priv/static/assets/phoenix_kit.js`):
`group` config. Tables sharing a group name can exchange items
via cross-container drag (catalogue's "items move between
categories" use case). When unset, single-container reorder
only — behavior unchanged.
`evt.from !== evt.to`. Cross-container payloads carry
`moved_id` plus the source container's scope attrs prefixed
with `from*`, alongside the destination's regular scope attrs.
Push routes through the destination's `data-sortable-event` so
the LV handler is co-located with the table the item ended up
in.
attrs (camelCase via DOMStringMap) into a `{key: value}` map.
The prefix-length check ensures a bare `data-sortable-scope`
attr can't masquerade as scope data.
value, e.g. corrupt scope or missing source container after a
fast unmount, logs to `console.error` instead of leaving
SortableJS half-initialized).
assumption — if a third-party script injects sortable-item
nodes, the LV handler should reject unknown IDs server-side.
`TableCardView` hook (`priv/static/assets/phoenix_kit.js`):
Added `updated()` callback that re-applies the user's saved view
mode after every LV-driven render. Without it, morphdom resets the
runtime `md:hidden` class toggles back to template defaults on
each diff (e.g. a SortableJS drop fires a reorder event, the LV
re-renders, and the card view snaps back to the table view). The
hook now caches `currentMode` on mount + every toggle/event and
replays it in `updated()`.
`<.draggable_list>` (`components/core/draggable_list.ex`):
New `:draggable` boolean attr (default `true`). When false,
the container renders without `phx-hook="SortableGrid"` and
items skip the `cursor-grab` styling — useful when a list is too
short to reorder (length ≤ 1) so the affordance doesn't lie.
Default `true` keeps every existing caller unchanged.
`<.table_default>` (`components/core/table_default.ex`):
Four new attrs — `:on_reorder`, `:reorder_scope`,
`:reorder_group`, `:item_id` — wire the card-view container
as a `SortableGrid` hook target when set. Each rendered card
gets `class="sortable-item cursor-grab active:cursor-grabbing"`
(mobile or desktop click) gets the same DnD experience as the
table view. `:item_id` defaults to `& Map.get(&1, :uuid)` so
the conventional schema shape works zero-config.
A footer row joins the card-actions slot when sortable: drag-handle
icon (`hero-bars-3` with cursor-grab styling,
`gettext("Drag to reorder")` tooltip) bottom-left, action
buttons bottom-right. `use Gettext, backend: PhoenixKitWeb.Gettext`
added to match the convention in `flash.ex` / `input.ex` /
`integration_picker.ex`.
A private `build_sortable_scope_attrs/1` helper translates a
`%{key => value}` scope map into
`[{"data-sortable-scope-key" => value}, ...]` tuples, spread
via `{@reorder_scope_attrs}` in HEEX (Phoenix's
`attributes_escape/1` applies, so untrusted-input callers
don't introduce an XSS path).
AGENTS.md TODOs section (`af35b067`)
C12 triage on the V108 + DnD work surfaced that there are no tests
under `test/phoenix_kit_web/components/core/` — the directory
doesn't exist. Several components there have grown to non-trivial
attr surfaces that warrant rendered-HTML coverage, but landing a
single fixture in an empty test dir alongside a feature PR muddles
the diff.
Recorded the gap as a TODOs section at the bottom of `AGENTS.md`
so a future component-coverage sweep has the inventory ready
(`<.draggable_list>` :draggable branches, `<.table_default>`
:on_reorder / :reorder_scope / :reorder_group / :item_id branches,
the form primitives, plus `<.flash>` / `<.modal>` if their
complexity has grown).
Triage outcome (C12 + C12.5 deep dive)
Three Explore agents ran against the diff. Most findings turned
out to be agent overreach:
alarm**. The pattern is consistent with all 107 prior
migrations; `prefix` is host-app-controlled at migration time,
not user input.
**false alarm**. HEEX omits `nil` attrs.
alarm**. Phoenix HEEX `{@list_of_tuples}` spread routes
through `attributes_escape/1` which escapes values.
Per V107's docstring, schema-only changes are implicitly verified
by `test_helper.exs` running all migrations on boot; only
backfill logic gets a dedicated test, and V108 has none.
of scope**; landing a single fixture in an empty test dir is
awkward. Surfaced via the new AGENTS.md TODOs section.
Real findings, all closed in this batch:
now log to `console.error`.
docstring at the hook.
`use Gettext` to `<.table_default>`, wrapped the title.
Companion PR
The downstream consumer is the in-flight
`mdon/phoenix_kit_entities`
work which adds DnD to the entity definitions list and the
DataNavigator records list. Until this core PR merges + a release
cuts, the entities suite fails on `column phoenix_kit_entities.
position does not exist` because the local schema has the
`:position` field but the Hex-pinned core's migration chain stops
at V107.
Verification
issues — 0 net change vs pre-PR; the one pre-existing format
violation in `integrations/providers.ex` is intentionally
left out of this diff)
6 tests, 0 failures (2 from the original `dynamic_children_fn
type` describe block + 4 new dispatch tests via the
`invoke_dynamic_children_for_test/3` delegate)
`phoenix_kit_parent`'s migration runner: up adds the three
columns, down drops them, marker comment flips both directions.
Test plan
via a parent app running V108 locally