Skip to content

Integrations V114: uuid-only storage keys + picker UX + form fixes + C12 sweep#536

Merged
ddon merged 8 commits into
BeamLabEU:devfrom
mdon:dev
May 12, 2026
Merged

Integrations V114: uuid-only storage keys + picker UX + form fixes + C12 sweep#536
ddon merged 8 commits into
BeamLabEU:devfrom
mdon:dev

Conversation

@mdon
Copy link
Copy Markdown
Contributor

@mdon mdon commented May 12, 2026

Summary

Quality sweep on the Integrations subsystem and the core
IntegrationPicker / integration_form LV. The architectural change
is V114 — collapsing the per-row phoenix_kit_settings.key
from the composite integration:<provider>:<name> shape to just
the row's uuid. Lifts both name restrictions (regex + uniqueness)
that were storage-layout artifacts, not product decisions: any
non-empty string is now a valid connection name, duplicates within
a provider are allowed.

(V113 was occupied upstream by the Tessera migration. This PR's
migration was originally numbered V113 locally; renumbered to
V114 during rebase per the workspace convention.)

PR follow-up(s)

Quality sweep

Storage refactor (V114)31ab4ad6 (renumbered post-rebase)

  • add_connection/3: dropped the [a-zA-Z0-9][a-zA-Z0-9\-_]*
    regex AND the per-provider uniqueness check. UUIDv7 is generated
    up-front and embedded directly in both the row's uuid column
    and the key column.
  • rename_connection/3: rewrites only the JSONB name field. No
    more key column rewrite, no uniqueness check, no regex.
  • Read sites (get_integration_by_uuid/1, list_connections/1,
    load_all_connections/1) source provider+name from JSONB; the
    list helpers also expose :date_added so UI callers can render
    "Created N ago" without a second lookup.
  • provider:name string lookups become first-match semantics
    (names aren't unique anymore). Read-shim contract preserved for
    legacy migrate_legacy/0 callsites.
  • log_activity takes explicit (provider, name) so audit rows
    carry human-readable names — parsing the key would have stamped
    a uuid string into metadata.connection.

Migration (V114)

  • One UPDATE walks every integration:%-keyed row, backfills
    missing value_json -> 'name' / 'provider', ensures module = 'integrations', and rewrites key = uuid::text. Legacy V0-shape
    keys without a :name segment fold to name = "default".
  • down/1 rewrites back to composite shape; collision suffix
    -<8-char uuid> on duplicate (provider, name) pairs (lossy by
    necessity).
  • 9 round-trip tests in v114_test.exs covering the up rewrite,
    name backfill, JSONB-name preservation, V0 keyless shape →
    "default", module stamping, non-integration row skip,
    idempotence, and down-side collision suffixing. One real bug
    caught by the test (V0 keyless integration:google was being
    treated as name="google" instead of "default"; fixed via
    split_part SQL).

Form-clear preservation28cb0b59

  • create_connection + save_form_with_rename error branches
    now preserve :new_name + :form_values (mirroring the
    existing test_credentials_dry_run/2 path). Pre-fix, hitting
    :empty_name on submit wiped the api_key the operator just
    typed.
  • Dropped the dead :already_exists / :invalid_name error
    branches (those tuples no longer fire post-V114).
  • Template: removed the now-incorrect "Letters, digits, hyphens,
    and underscores. Must be unique per provider." hint and the
    <provider> Connection label.

IntegrationPicker4106ce33

  • Card subtitle: priority external_account_id → masked
    credential tail (first 8 + … + last 4 for any of api_key /
    bot_token / access_key; ••• for keys under 14 chars).
  • Age line under subtitle using shared <.time_ago>.
  • Status badge: distinct labels + colours for each of the four
    canonical statuses (connected → green "Connected",
    error → red "Auth failed" with validation_status tooltip,
    configured → yellow "Not tested", disconnected → grey "Not
    connected"). Aligns with the Endpoints list LV's vocabulary.
  • Provider icon + display name now auto-resolve via
    Integrations.Providers.get(data["provider"]) — callers no
    longer have to pre-attach a :provider struct. Dead
    is_map(conn[:provider]) / is_binary(conn[:provider_key])
    branches removed.
  • Provider-name badge hides when the picker is filtered to a
    single provider (provider attr set) — both real callsites do
    this, making the badge redundant.
  • Click feedback: phx-click-loading:opacity-60 +
    phx-click-loading:pointer-events-none dims the clicked card +
    blocks rapid re-clicks during the LV round-trip; daisyUI
    loading-spinner swaps in for the status badge during the
    same window. 33 new component spec tests.

C12 sweep findingsbe9efc43, 22ee0865

  • phx-disable-with added to Save / Test Connection / Disconnect /
    Delete buttons on integration_form.html.heex. Pre-fix, a
    double-click + slow network could submit two save requests or
    spawn parallel HTTP probes; data-confirm is friendlier UX but
    not a substitute.
  • Narrowed Integrations.validate_connection/2's rescue e -> to
    three expected exception classes (DBConnection.OwnershipError,
    Postgrex.Error, Req.TransportError). Genuine logic bugs
    (KeyError, MatchError, etc.) now surface to the supervisor +
    Logger.error instead of getting swallowed under the generic
    "validation failed" message.
  • Doc-only security note on Integrations.authenticated_request/4
    warning that the attached Bearer token leaks if the caller's
    url parameter comes from unvalidated user input. Internal
    callers (OpenRouterClient, OAuth refresh, userinfo) are safe;
    the AI module's Endpoint.validate_base_url/1 schema guard
    covers the operator-supplied case. The doc spells out that new
    callsites that take URLs from anywhere else need their own
    allowlist.

Permissions module_label("db") fix — 0d24c35e

  • Pre-existing failure on upstream/dev: when the external
    phoenix_kit_db module isn't loaded (core's own test VM doesn't
    pull in feature modules), Permissions.module_label("db") falls
    through to String.capitalize("db") = "Db", but the test
    asserts "DB" (the registered shape from
    phoenix_kit_db.permission_metadata/0).
  • db was extracted into its own module but core still references
    the key (phoenix_kit_web/users/auth.ex keeps a
    {"db", "/admin/db"} route). Same pattern as if "shop" were
    extracted later: the canonical label / icon / description belong
    in core's @core_* maps so the display is correct regardless of
    whether the external module is installed.
  • Added three parallel entries (@core_labels "db" => "DB",
    @core_icons "db" => "hero-server-stack", @core_descriptions
    "db" => "Database explorer and schema inspection") mirroring
    phoenix_kit_db.permission_metadata/0 verbatim — a host app
    loading both core and the external module sees the same values
    regardless of which lookup wins.
  • Folded in to keep the post-rebase baseline green at 0 failures.

Upstream fixture fix — 3c9ca9f5

  • The rebase brought in upstream's V113 (PR Updated the media browser with tessera #534, Tessera) which
    added the phoenix_kit_files_user_or_parent_check CHECK
    constraint requiring user_uuid IS NOT NULL OR parent_file_uuid IS NOT NULL. Existing fixtures in storage/scope_test.exs,
    media_browser_scope_test.exs, and media_browser_test.exs
    insert phoenix_kit_files rows with neither — fixture pattern
    predates V113. Reproduced on clean upstream/dev (93f242b3)
    via separate worktree, so confirmed not caused by this PR.
  • Fix: each create_file!/1 now stamps user_uuid via a memoised
    ensure_user!/0 helper that registers an owner user once per
    test process. The user identity is incidental to every
    assertion in these tests; it exists only to satisfy the V113
    CHECK. Folded into this PR rather than a follow-up so the
    baseline mix test is clean — otherwise the next sweep would
    be unable to distinguish new regressions from upstream's
    pre-existing red.

Verification

Check Result
mix compile --warnings-as-errors clean
mix credo --strict same pre-existing counts as upstream/dev (no new findings)
Targeted tests on changed files 162 / 162 pass
Full mix test 1229 tests, 0 failures. The 23 upstream Tessera-fixture failures (introduced by V113's phoenix_kit_files_user_or_parent_check CHECK constraint) are fixed in 3c9ca9f5 — each create_file!/1 fixture stamps user_uuid via a memoised ensure_user!/0 helper. The remaining Permissions.module_label("db") failure (which existed on clean upstream/dev, unrelated to this PR's primary scope) is fixed in 0d24c35e"db" => "DB" added to @core_labels / @core_icons / @core_descriptions so core's display is correct even when phoenix_kit_db (which registers the same shape) isn't loaded. Targeted tests on this PR's changed files: 162 / 162.

Test plan

  • V114 migration round-trip + idempotence + edge cases (9 tests)
  • add_connection accepts spaces / punctuation / duplicates / leading symbols (4 tests)
  • rename_connection accepts arbitrary names + duplicate targets (5 tests)
  • list_connections + load_all_connections expose :date_added (6 tests)
  • Form-clear preservation on empty-name create error (1 test)
  • IntegrationPicker component spec — subtitle priority + masked
    credential + age + provider auto-resolve + 5 status branches
    + filter-by-provider + search threshold + empty state +
    deleted-card warning + click-action dispatch (33 tests)
  • mix precommit
  • for i in {1..3}; do mix test --exclude integration; done
    (stable on non-DB tests)

Renaming V113 → V114 across the migration, the test, the
@current_version constant, and the moduledoc changelog entry.

mdon added 8 commits May 12, 2026 17:52
Pre-V113 each integration row's storage key was
`integration:<provider>:<name>` — that composite baked the
human-chosen name into the row's identity, forcing names to match a
strict regex (`[a-zA-Z0-9][a-zA-Z0-9\-_]*`) and to be unique within
a provider. Both restrictions were storage-layout artifacts, not
product decisions: operators wanted "My Company Drive" and a second
OpenRouter account also called "personal" without the system pushing
back.

V113 collapses the storage key to just the row's uuid. The `module`
column (already stamped `"integrations"` via `@settings_module`)
becomes the sole row-class discriminator; provider and name live
purely in `value_json`. The data migration walks every row whose
`key LIKE 'integration:%'`, backfills missing `provider`/`name`
JSONB fields from the old composite key (legacy V0 keyless rows
fold to `name = "default"`), and rewrites `key = uuid::text` +
`module = "integrations"`. `down/1` rewrites back to the composite
shape, collision-suffixing `-<short uuid>` on duplicate (provider,
name) pairs.

API changes in `integrations.ex`:

  * `add_connection/3`: drops the regex + uniqueness checks; only
    `:empty_name` (after trim) remains. Generates the UUIDv7
    up-front via `put_change(:uuid, ...)` so the row's uuid
    matches the embedded key.
  * `rename_connection/3`: just rewrites JSONB `name`; no key
    rewrite, no uniqueness check.
  * Read sites (`get_integration_by_uuid/1`, `list_connections/1`,
    `load_all_connections/1`) source provider+name from JSONB
    instead of parsing the storage key. `list_connections/1` and
    `load_all_connections/1` also expose `:date_added` so UI
    callers can render "Created N ago" without a second lookup.
  * `provider:name` string lookups (`get_integration/1`,
    `get_credentials/1`, `find_uuid_by_provider_name/1`,
    `resolve_to_uuid/1`) shift to first-match semantics — names
    are no longer unique, but legacy migrate_legacy callsites
    still need the input shape to work.
  * Internal `save_integration` rewritten to take the Setting
    struct + JSONB and update via `Repo.update` directly.
  * `settings_key/1`, `parse_provider_name/1`,
    `do_rename_connection`, `rename_setting_row` removed.
  * `log_activity` takes explicit `(provider, name)` so audit
    rows carry the human-readable name post-V113 (parsing the key
    would have stamped a uuid string into `metadata.connection`).

`@current_version` bumped to 113. V113 entry added to the
migrator's moduledoc changelog.

Test coverage:

  * V113 migration round-trip test pins the rewrite, name
    backfill, V0 keyless-shape handling, `module` stamping,
    idempotence, and the down-side collision suffixing. (Caught
    one real bug — the V0 keyless case treated `name="google"`
    instead of `"default"` until the SQL used `split_part`
    rather than `position`.)
  * Integration suite updated to drop the `settings_key/1`
    assertions, add positive tests for arbitrary names + duplicates,
    pin the new `:date_added` field, and rewrite the storage-key
    invariants in terms of "key = uuid".
`create_connection` and `save_form_with_rename`'s error branches
used to set `:error` and forget the typed values, so the api_key
input re-rendered with `value=""` after a blank-name or other
validation rejection. Operator saw the error AND a cleared form,
had to re-type the credential. The fix mirrors the existing
`test_credentials_dry_run/2` path — error branches now also assign
`:new_name` + `:form_values` via `extract_setup_attrs/2`.

Removed the now-dead branches that handled `:already_exists` and
`:invalid_name` (those error tuples no longer fire after the V113
storage refactor lifted both restrictions). The unused
`lookup_connection_uuid/2` helper goes with them.

Template: dropped the "Letters, digits, hyphens, and underscores.
Must be unique per provider." hint under the Connection Name input
— the rule it described no longer applies. Placeholder text updated
to suggest more natural example names (Main, Personal, My Company
Drive).

Test: new regression "blank-name create error preserves typed
credential values in the form" asserts that an `:empty_name` error
re-renders with the typed api_key intact.
Five UX improvements to the connection cards rendered by
`<.integration_picker>` (used by the AI endpoint form and document
creator OAuth settings).

  1. Card subtitle now disambiguates same-named connections.
     Priority: OAuth `external_account_id` (e.g. "user@gmail.com")
     when present, otherwise the masked credential tail (first 8 +
     `…` + last 4 of whichever of api_key / bot_token / access_key
     exists; short keys < 14 chars fully mask to `•••`). Empty when
     the row has neither — silences a redundant blank line.

  2. "Created N ago" line under the subtitle, using the shared
     `<.time_ago>` hook so the value stays live without a server
     round-trip. The new `date_added` field on the connection map
     (exposed by V113's `list_connections/1` / `load_all_connections/1`
     update) powers this.

  3. Status badge gains diagnostic detail instead of collapsing
     every non-`"connected"` state into a single "Not connected":

       | status         | label           | colour          |
       | -------------- | --------------- | --------------- |
       | "connected"    | "Connected"     | badge-success   |
       | "error"        | "Auth failed"   | badge-error     |
       | "configured"   | "Not tested"    | badge-warning   |
       | "disconnected" | "Not connected" | badge-ghost     |

     For `"error"` rows the `validation_status` string hovers as
     the `title` tooltip so operators see WHY without clicking
     through. Aligns with the Endpoints list LV's existing
     vocabulary.

  4. Provider icon + display name now auto-resolve via the
     `PhoenixKit.Integrations.Providers` registry — callers no
     longer have to pre-attach a `:provider` struct. The
     `is_map(conn[:provider])` and `is_binary(conn[:provider_key])`
     branches of the old `conn_provider_key/1` chain were dead
     across every live caller; collapsed to `conn.data["provider"]`.
     Provider-name badge hides when the picker is filtered to a
     single provider (`provider` attr set) — both real callsites
     do this, making the badge redundant.

  5. Instant click feedback. `phx-click-loading:opacity-60
     phx-click-loading:pointer-events-none` dims the clicked card
     the moment LiveView applies its in-flight class (no LV state
     needed). A daisyUI loading spinner replaces the status badge
     during the round-trip via `phx-click-loading:hidden` /
     `phx-click-loading:inline-flex` toggles. Bridges the
     perception gap between click and server-side state catching
     up (especially while the new validate-then-fetch flow does
     its provider auth round-trip).

Also dropped the dead `String.starts_with?(key, provider <> ":")`
branch in `filter_by_provider/2` (composite shape no longer
exists post-V113).

New spec file: 33 tests covering selection checkmark colours,
subtitle priority + credential masking, age rendering, provider
auto-resolution + filter-mode badge gating, status badges + tooltip,
filter by provider, search input visibility threshold, empty state,
deleted-card warning, and click-action dispatch (single vs multi
select). The first round caught a UX bug (provider-name badge
showing for filtered single-provider picks) and the bot_token
mask suffix length, both fixed before the spec landed.
… review

`CLAUDE_REVIEW.md` for PR BeamLabEU#526 (add selected-card indicator to
SortableGrid styles) opens with `APPROVE`. Two NITPICKs and one
IMPROVEMENT-LOW, no BUG findings. Documenting their status:

  * **NITPICK** — redundant `!important` on `background-color` —
    already fixed post-merge in commit `3521fa2f` ("Remove redundant
    !important from selected-card indicator"). Current rule at
    `priv/static/assets/phoenix_kit.js:170` doesn't carry it.
  * **NITPICK** — `:has()` selector matches any descendant checkbox,
    not specifically the bulk-select one. Benign for every current
    consumer (only `phoenix_kit_catalogue` uses sortable card view,
    and its cards have exactly one checkbox). The review explicitly
    calls it "ergonomic, not correctness" — skipped, with the
    forward-looking guidance preserved for whenever a second
    consumer trips it.
  * **IMPROVEMENT-LOW** — no automated coverage. Already a tracked
    TODO in `phoenix_kit/AGENTS.md` ("Component test coverage for
    `phoenix_kit_web/components/core/`"). Defer to that future
    sweep per the reviewer's own suggestion.

No code changes — just folding the existing fix-commit and the two
forward-looking notes into the canonical FOLLOW_UP.md format. Phase
1 triage discovered no actionable items beyond what was already
addressed.
C12 BeamLabEU#1 agent flagged that the Save / Test Connection / Disconnect /
Delete buttons in `integration_form.html.heex` all lacked the
`phx-disable-with` guard. Playbook calls this out explicitly:
"phx-disable-with on every phx-submit form button AND every
phx-click async/destructive button (delete, restore, refresh,
export, etc.)."

Without it, double-clicks on the Save button can submit two save
requests, the Test Connection button can spawn parallel HTTP probes,
and the destructive buttons (Disconnect / Delete) can fire twice
during the `data-confirm` modal's dismiss window — even though the
modal slows the operator down, a confirm + double-click race is
still reachable. `phx-disable-with` is the load-bearing protection
for async LV operations; `data-confirm` is friendlier UX but not a
substitute.

Added to all four:

  * Save — `phx-disable-with={gettext("Saving…")}`
  * Test Connection — `phx-disable-with={gettext("Testing…")}`
    (alongside the existing `disabled={@testing}` state guard)
  * Disconnect — `phx-disable-with={gettext("Disconnecting…")}`
  * Delete — `phx-disable-with={gettext("Deleting…")}`

LV form tests (24/24) still pass — none of them double-click these
buttons today, but the regression guard is in place.
…L trust

Two adjacent quality findings from the C12 sweep on the integrations
context.

**Narrow rescue in `validate_connection/2`.** The function caught
every exception under `rescue e ->` and returned a generic
"Validation failed unexpectedly" string. That swallowed genuine
logic bugs (KeyError, MatchError, etc.) under a misleading
user-facing message instead of letting them surface to the
supervisor where Logger.error + Sentry/AppSignal would actually see
them. Narrowed to the three exception classes that are real
failure modes on this path:

  * `DBConnection.OwnershipError` — sandbox checkout race in
    tests that hit this path from an async-shared connection.
  * `Postgrex.Error` — DB outage / table-missing mid-flight
    during helpers like `log_activity` or future provider-DB
    lookups.
  * `Req.TransportError` — belt-and-braces; should already be
    returned as `{:error, _}` by `check_http/2`, but keep the
    catch for defence in depth.

Everything else now bubbles up. Documented inline why.

**Doc-only security note on `authenticated_request/4`.** The
function attaches the integration's Bearer token to every dispatched
request. If a caller passes a URL from unvalidated user input, the
token leaks to whatever host that URL points at. Internal usage
(`OpenRouterClient.fetch_models/2`, OAuth refresh, userinfo lookups)
builds URLs from the Providers registry, which is hardcoded and
safe. The AI module's schema-level `validate_base_url/1` guard
covers the operator-supplied `base_url` case. New callsites that
take URLs from anywhere else need their own guard — added a
"Security — `url` must come from a trusted source" paragraph to
the `@doc` block so a future reader knows the contract.

No behavioural change for current callers. Compile clean.
Upstream's V113 (PR BeamLabEU#534, Tessera tile media) added the
`phoenix_kit_files_user_or_parent_check` CHECK constraint requiring
`user_uuid IS NOT NULL OR parent_file_uuid IS NOT NULL`, but the
existing `create_file!/1` fixtures in three test files insert
`phoenix_kit_files` rows with neither column set — fixture pattern
predates V113. Result: 23 failures across:

  * `test/integration/storage/scope_test.exs` (17)
  * `test/integration/media_browser_scope_test.exs` (4)
  * `test/integration/phoenix_kit_web/components/media_browser_test.exs` (2)

All three reproduce on clean `upstream/dev` (commit `93f242b3`) with
zero changes from this PR — verified via a separate `git worktree`.

Fix: each `create_file!/1` now stamps `user_uuid` from a new
memoised `ensure_user!/0` helper. The helper registers an owner
user once per test process (via `PhoenixKit.Users.Auth.register_user/1`
+ `Process.put/2`) and reuses the uuid on subsequent calls — keeps
the existing call sites unchanged, avoids the unique-email collision
that would fire if we registered fresh per file. The user identity
itself is incidental to every assertion in these tests; it exists
only to satisfy the V113 CHECK.

Folded into this PR (rather than a follow-up) since the rebase
brought the broken upstream tests into our `mix test` baseline, and
leaving them red would mask future regressions in the next sweep.
Same fix-shape would apply on `upstream/dev` directly — if/when
alexdont or @timujinne ship a patch there, this commit should merge
cleanly with theirs (the bodies of `create_file!/1` are otherwise
unchanged).

Verification: full `mix test` 1229 tests / 1 failure (down from 24
on the unfixed baseline). The lone remaining failure is the
pre-existing `Permissions.module_label("db")` returning `"Db"`
instead of `"DB"` — flagged in an earlier session, unrelated.
`Permissions.module_label("db")` used to fall through to
`String.capitalize("db")` = `"Db"` in environments where the
external `phoenix_kit_db` module wasn't loaded — including core's
own test VM, which doesn't pull in feature modules. The permissions
test at `permissions_test.exs:104` asserted `"DB"` (the registered
shape from `phoenix_kit_db.permission_metadata/0`) and failed
deterministically with `Assertion with == failed: left "Db", right
"DB"`.

`db` was extracted into `phoenix_kit_db` per the workspace AGENTS.md,
but core still references the key — `phoenix_kit_web/users/auth.ex`
keeps a `{"db", "/admin/db"}` route entry. Same as `"shop"` would
need if the e-commerce module is extracted later: the canonical
label / icon / description belong in core's `@core_*` maps so the
display is correct regardless of whether the external module is
installed.

Added three parallel entries (mirroring the four other core keys'
shape):

  * `@core_labels` — `"db" => "DB"`
  * `@core_icons`  — `"db" => "hero-server-stack"`
  * `@core_descriptions` — `"db" => "Database explorer and schema
    inspection"`

Mirrors `phoenix_kit_db.permission_metadata/0` verbatim so a host
app loading both core and the external module sees the same values
regardless of which lookup wins.

Verification: 1229 tests, 0 failures (down from 1 with the
`module_label` failure). Targeted permissions_test.exs 55 / 55.

Folded into this PR to keep the post-rebase `mix test` baseline
green so future regressions on the same key are distinguishable
from "known red."
@ddon ddon merged commit 30a8ae2 into BeamLabEU:dev May 12, 2026
ddon pushed a commit that referenced this pull request May 12, 2026
- V114 module + tests + picker comment: post-V113 → post-V114 (rebase
  rename leftovers)
- validate_credentials/2: narrow rescue to match validate_connection/2
  (DBConnection.OwnershipError, Postgrex.Error, Req.TransportError)
- validate_connection/2 rescue comment: drop misleading record_validation
  mention; name log_activity as the actual Postgrex source
- integration_picker.ex filter_by_search: rename shadowing `name` var
  to `provider_name` in pattern match
- integration_form OAuth Connect button: add phx-disable-with for parity
  with the rest of the C12 sweep
- v114_test.exs: add 3-row down-collision test (exactly one plain key,
  N-1 distinct suffixed keys, no collision)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 12, 2026
20 findings across BUG / IMPROVEMENT / NITPICK. Tier 1 follow-ups
(OAuth state phantom activity, validate_credentials rescue) and Tier 2
(docstring drift, Permissions "db" precedence, V114 SQL guard, form
component migration). The trivial fixes (#2 doc rename, #3 rescue
narrow, #7 3-row test, #8 comment, #11 shadow, #14 disable-with)
are addressed in the preceding commit; remaining items are flagged
for a future sweep.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 12, 2026
Replaces the `<next-sha>` placeholder with `a99fd681` and undoes
two collateral replacements (the previous `replace_all` on the
literal string "follow-up" caught two unrelated occurrences).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 12, 2026
7 findings: 1 BUG-LOW (my carry-over from PR #536 — V114 down SQL
suffix collision because UUIDv7's first 8 chars are timestamp not
random), 5 IMPROVEMENT-LOW (V116 no-op conditional, prefix_str drift,
self-loop CHECK, defensive table_exists, component test TODO widening),
1 IMPROVEMENT-LOW component (sortable_handle typo-safety).

Strengths: mirrors V103 catalogue pattern, idempotent, no-cascade
trade-off documented, sortable_handle is purely additive (JS already
supported data-sortable-handle), PR description honest about pre-
existing test failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 12, 2026
Code (PR #538 #1 — carry-over from PR #536 follow-up):

- V114 down SQL: switch the collision-suffix source from
  `substring(uuid::text from 1 for 8)` (UUIDv7 timestamp prefix — same
  millisecond ⇒ identical prefix ⇒ duplicate suffixed keys when two+
  rows collide on (provider, name)) to `substring(uuid::text from 25
  for 8)` (the post-variant random tail, 32 bits of entropy ⇒ 1-in-4B
  collision probability per pair). Mirrored in `run_down!` in
  `v114_test.exs` since the test duplicates the SQL.
- V114 moduledoc updated to spell out the suffix source and why the
  timestamp prefix was wrong.

The fix is forward-compat: systems that already ran V114.down get the
old (potentially-collided) keys; fresh installs and any future
rollback get the corrected behavior.

Docs (PR #538 #6):

- AGENTS.md TODO entry for `<.draggable_list>` test coverage widened
  to call out the new `:sortable_handle` axis: three branches
  (`:draggable=false`, `:draggable=true + sortable_handle=nil`,
  `:draggable=true + sortable_handle=".pk-drag-handle"`).

Plus a one-line `mix format` normalization in V116 (multi-line
`execute(...)` collapsed to single line).

mix precommit: compile → format → credo --strict (0 findings) →
dialyzer (160 errors all skipped) clean.

Deferred to maintainer: #2 / #3 / #5 (cosmetics on deployed
migrations), #4 (DB-level self-loop CHECK — needs a V117), #7
(`sortable_handle` typo safety — design call on boolean shape vs
JS warning).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 12, 2026
CHANGELOG: covers PRs #536 (Integrations V114 + IntegrationPicker +
form fixes), #537 (V115 annotations + Etcher overlay + AnnotationComposer),
#538 (V116 entity_data parent_uuid + sortable_handle), plus all
post-merge review fixes folded into dev between merges. Organized
Added / Changed / Fixed / i18n to match the 1.7.108 entry's style.

AGENTS.md: the "entries written by the maintainer, not agents — flag
the gap and stop" line was filling context with a false rule. Replaced
with reality: agents draft the entry against the bumped @Version
heading.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to 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