Uh oh!
There was an error while loading. Please reload this page.
Add cross-module mentions, one canonical display name, and stop publishing email addresses - #692
Merged
Conversation
A disabled tbody kept emitting data-sortable-event/-items/-handle — inert without the hook, but the literal ".pk-drag-handle" string broke consumers' honest substring assertions (phoenix_kit_projects' DnD load-more-truncation test refutes "pk-drag-handle" while non-draggable) and shipped meaningless attributes to the client. The component's own doc already promised "renders identically but without drag behavior". All four sortable attrs now key off @enabled; the disabled-state test pins their absence.
The activity→notification bridge resolves ONE target_uuid per entry; events whose audience is a SET (project members, watchers) had no path that respects the whole machinery. fan_out_from_activity/2 re-routes a committed entry per recipient — prefs, per-channel routing, digest cadences, and the actor self-skip all apply — WITHOUT inserting extra activity rows: the feed keeps one canonical entry and deliveries still key on its committed uuid. Recipients dedupe; results return in order. First consumer: phoenix_kit_projects' event notifications (all project members). Tested: per-recipient fan-out with actor skip + zero new feed rows, and per-user pref honoring.
Feature modules that reference users by UUID (memberless orphans, ownership succession) need a pre-delete signal. delete_user now calls each injectable module exporting before_user_delete/1 with the user's uuid before the delete transaction; per-module rescue/catch logging so one module can never block the deletion.
External modules' user_dashboard_tabs/0 entries carrying a live_view now generate their /dashboard/* routes in the authenticated route table, mirroring compile_module_admin_routes — the :user_dashboard context clause in tab_callback_context/1 existed but nothing called it. Tabs without live_view (the hardcoded shop/billing/tickets blocks) are untouched. First consumer: phoenix_kit_projects' member surface.
…tics Panel follow-ups: catch kind/reason so a throwing hook cannot abort deletion; document that hooks deliberately run outside the delete transaction (their writes must survive; rare post-hook delete failures are admin-recoverable).
Final-panel find: without :peer_data in connect_info, get_connect_info(socket, :peer_data) is nil on connected mounts, so per-IP abuse buckets (the projects portal) collapse into one shared bucket. Host apps should mirror this in their own endpoints.
The old component could never have worked: its class attribute was a
plain string with un-interpolated \#{} calls to functions that don't
exist, and it depended on .accordion-* CSS no host defines. Rebuilt on
the native <details> + daisyUI collapse with an optional server-tracked
mode (open + toggle_event) — without it, morphdom re-applies the
server-rendered state on every patch and a section the user opened
slams shut mid-use (found on the projects creation form, its first
consumer).Make the client the single authority on the details open state:
JS.ignore_attributes("open") on mount stops morphdom from resetting
a section the user toggled, and with no server round trip fighting
the native toggle, daisyUI 5's ::details-content height transition
finally runs — sections slide open and shut instead of snapping.
The server-tracked mode (toggle_event/toggle_value) is gone; open
now sets only the initial state, and pages open a section from
elsewhere via JS.set_attribute.Closing an expanded collapse near the bottom of a long page shrank the document, the browser clamped the scroll position to the new maximum, and the viewport snapped upward under the reader. Pad the document with a body spacer just before the collapse so it cannot become shorter than their current position: they stay looking at the section they just closed, with blank space below the fold. Each pixel they scroll up releases a pixel of spacer — height leaving from entirely below the viewport is invisible — so the page works its way back to its natural length without ever moving underfoot. Measured on the summary activation rather than the toggle event, since by then the element may already be collapsed (prefers-reduced-motion), and released against the reader's scrolling rather than the live document height, which mid-collapse still contains the content on its way out and reads as though the spacer were unnecessary.
The held height was going on a body-level div, below <main> and outside the drawer entirely. The admin sidebar is a sticky grid item, so it can only extend as far as its grid row: the row kept its old height, the sidebar stopped dead at the previous page bottom, and the blank area below showed bare body with a sidebar cut off mid-item. Give the layout an explicit pad as the last child of the content column and let the keeper prefer it — growing that cell grows the grid row, so the sticky sidebar and the page background carry on through the held space. It is server-rendered so morphdom keeps it, with style handed to the client so the height survives patches. Layouts without the marker still get the injected body spacer, which holds the scroll position the same way but leaves the blank area outside the app chrome.
When a click in one place changes something in ANOTHER — picking a
starting point that rewrites a capability checklist, enabling an
extension that adds a permission row — the change is real but silent,
because it lands inside a collapsed section the reader isn't looking at.
push_event(socket, "pk-flash", %{ids: [...]}) now briefly highlights those
sections. LiveView dispatches pushed events on window as phx:<name>, so
this needs no hook and no per-element wiring: any module can say which
sections its click just changed.
Restarts the animation when the same section flashes twice in a row —
re-adding a class an element already has does nothing, so a rapid second
change would have looked like nothing happened. Honors
prefers-reduced-motion: same outline, no pulse.Highlighting a whole section tells someone already looking at it nothing, and highlighting a closed section raises a question it can't answer: the reader opens it and everything looks the same. So granularity follows what the reader can see. Section open: flash the changed ROWS. Section closed: flash the section, remember the rows for ten seconds, and if they open it in that window the rows flash then. A change touching more rows than six flashes the section instead — past a handful, individual highlights read as strobing rather than as information. The replay waits a beat after the toggle so the highlight doesn't play against content that is still zero-height mid-reveal.
Four AIs reviewed the first version. Two of their corrections changed the design, and both were right. THE SERVER STOPPED DECIDING WHERE. It sent section ids plus row ids; it cannot know which sections are open, so it was guessing at a decision only the client can make. It now sends only WHAT changed. The client resolves each id, walks up to its [data-change-region], and picks: region open, highlight the rows; region closed, highlight the region. THE TIMER BECAME A MARKER. "Remember for 10 seconds" turns "what changed in here?" into a race against a stopwatch — one panelist wanted the deferral cut entirely on those grounds. A closed region now keeps a quiet marker until it is opened, whenever that is; opening replays the rows and clears it. That also covers the case a timer never could: a change that lands while the reader is scrolled somewhere else is not lost, because the marker is still there when they arrive. Pending rows are keyed by region rather than stashed on the DOM node, since LiveView can replace the node in between; the marker attribute is declared client-owned so morphdom can't drop it on the next keystroke. Also renamed. "Flash" collides with Phoenix's flash messages and names an animation rather than a capability. A closed-region change now also writes to a polite live region — the caller's own words, coalesced, never assertive, never repeated on replay — because a pulse tells a screen-reader user nothing. Still refuses to scroll, open anything, or move focus.
… looking Two things it got wrong. THE MARK ACCUMULATED INTO A LIE. It said "an event happened", so flipping between presets eventually decorated every section even when the state was back where it started and nothing differed. It now means "there is something here you haven't seen": the caller diffs against a per-region baseline, ChangeCue.clear/2 removes marks whose difference is gone, and a cued region reports itself with ChangeCue.seen_event() when the reader opens it, which is what resets that baseline. A round trip through the presets now ends with no marks at all. OPENING A LONG SECTION PLAYED THE HIGHLIGHT OFF SCREEN. The reader clicks, sees nothing, and the cue has cost them a click to tell them less than nothing. The replay now highlights what is actually in the viewport and watches the rest with an IntersectionObserver, so a row below the fold lights up when they scroll to it. Observers disconnect after thirty seconds — a section left open and never scrolled shouldn't hold them for the life of the page. Also: a target can now be sent grouped by the region that contains it, for the case that started this — a row that DISAPPEARS can't be found, and neither can the region around it, so an id alone cued nothing exactly when something visibly vanished.
Typing @ anywhere someone can write free text offers people; typing # offers records from every installed module. The picked result stores a self-contained token, indexes for reverse lookup, and renders resolved per viewer. The resolve half already existed — ResourceLinks turns (type, uuid) into a title and a deep-link for Activity and Comments, and the module behaviour has carried a resource_links/0 callback with zero adopters. This adds the three missing halves: search, storage, and a renderer that answers the permission question. Storage is a token in the text — @[user:uuid|Alice] — not a foreign key. Text gets copied between records, exported, kept in history, read with JavaScript off, and a module can be uninstalled entirely; a key survives none of that. The label is a snapshot of what the author saw, so a dead mention still reads as a sentence, and full-text search still finds "Alice" because "Alice" is in the column. A bare @alice or #launch is never linked, which keeps publishing's hashtags untouched and makes an unfinished token stay text rather than become a broken link. Rendering is per viewer, three ways: a live title and a real link if they may open it; the author's stored label, unlinked, if it's gone; and a redacted chip if it exists and isn't theirs to see. That last one shows that something is there without showing what — the Discord model — and clicking it asks the owners for access, because "you can't see this" is only useful with a next step. A refreshed title is never shown to someone who cannot open the record. Search fans out across modules in parallel behind a 250ms deadline: a module that is slow, raises, or has no index costs its own results and nothing else. Each module scopes to the searcher itself — a typeahead that offers records the searcher can't open has already leaked them. Visibility is asked separately, batched, because the searcher and the later reader are different people. Pings go out on the durable save only, diffed against what was already there, so editing text doesn't re-notify. Self-mentions are dropped, and nobody is told about a record they can't open. Delivery reuses the activity->notification bridge, so a ping obeys existing preferences and lands in Email/Telegram with no new plumbing. The typeahead is one hook attachable to any textarea (`mentions` on <.textarea>) because the real surface is ~70 plain fields, not the three rich editors. With JavaScript off the field behaves exactly as before — what the hook inserts is text the field could always have held. V163 adds the reverse index and the access-request queue. Neither target carries a foreign key: both point into ~28 optional packages' tables.
Five real problems, the first found by auditing the registry myself and the rest by an external adversarial pass. Visibility failed OPEN. A type whose handler declared no visible_resource_uuids/2 had all its uuids treated as visible, and six of the eight registered types declare none — posts, files, CRM contacts and companies, integrations, users. A token can be TYPED BY HAND into any textarea, so "this module didn't implement the check" was rendering live titles and working deep-links to any reader handed a uuid. It now fails closed: a type is mentionable only if it answers the visibility question. The single exception is `user`, because the @ typeahead already lists every pingable account to anyone in the admin area, so gating names protects nothing and would break every ping. A type nobody registered still renders as the author's words — nothing can resolve it, so there is no title to leak. The typeahead lost the admin override. search_reply passed only a user uuid, and a handler asking Authz.admin_all? gets an answer only from a scope — so a site admin silently got the membership-only list and every project they didn't personally belong to vanished from their own picker. The client built tokens by string concatenation, which produced something unparseable whenever a record's own name contained a `|` or a `]`. A project called "Q3 | Launch" stored a broken token that no renderer would link and no index would record. The server now builds and validates the token, falling back to a sanitised label rather than dropping the result. Delivery was a stamp, not a claim. Two saves of the same field racing — a double submit, two tabs — both saw an unnotified row and both sent. It now claims with WHERE notified_at IS NULL and only delivers what it won. XSS is covered by tests rather than by assumption: labels reach HEEx from attacker-controlled text, and the escaping is now pinned both ways.
The render path redacts a mention whose target the reader can't see. The notify path did not: it attached a 140-character preview of the source text to every ping, gated by a check that returned "yes" whenever the source type had no handler — which is every type except projects. So a comment on a private CRM contact reading "Acme — layoffs planned, cc @b" showed B a locked chip on screen and emailed them the sentence. The notify path was quietly overruling the render path, at the one point where content actually leaves the system. The check is three-valued now. An affirmative answer from the source type's own handler carries the preview. "Nothing can answer" — no handler, or a handler with no visibility check — still DELIVERS, because failing closed would drop every ping in every module that hasn't adopted the contract yet, and a notification whose link 403s is a dead end rather than a disclosure. What it no longer does is quote the text. Also pins the token-forgery case the same review raised: a record named `x] @[user:<ceo>|see this` would, under client-side concatenation, insert two valid tokens and ping the CEO as whoever picked it. Server-side construction already refuses those characters in a label; the test says so.
The site switch lands in Settings > Content Editor, next to the editor mode it sits beside conceptually. On by default: the feature is inert until someone types a trigger, and everything it stores degrades to plain text when it is off. The switch is checked at RENDER, not just in search — with mentions off a field is an ordinary textarea again, with no hook and no hint promising a feature that would silently return nothing. <.textarea mentions> and <.translatable_field mentions> are now the whole opt-in for a field, which is what makes the ~70 plain textareas reachable. translatable_field only honors it for type="textarea": a typeahead has nowhere to go in a single-line input. Also adds Mentions.to_markdown/2 for surfaces that render markdown rather than HEEx. One honest limitation: markdown output is sanitised afterwards, so a redacted mention there is plain text rather than a request-access button — the reader still learns something exists and isn't theirs.
A mention blanked to "No access" leaves a hole where a noun should be: the sentence stops naming the thing it is about. It now reads as the record it names, with a lock icon and no link — knowing what something is called is not the same as being let in — and clicking still asks the owners for access. The title is LIVE, resolved per render like any other mention, so a renamed record reads correctly for everyone rather than freezing at whatever it was called when someone typed it. That is a deliberate reversal of the earlier caution, and the caution is kept as an opt-in: `mentions_redact_titles` (Settings > Content Editor, off by default) withholds the title from a reader who cannot open the record, falling back to the label the author stored. Worth turning on where a record's NAME is the sensitive part; unnecessary noise where the access is. The linked record's type and id were already stored both ways — inside the token, and as target_type/target_uuid on the mentions row, which is what "what links here" reads.
`ImageProcessor.sanitize/3` decodes an image and writes a fresh one, so what gets stored is this encoder's output rather than the uploader's bytes. That is the difference between accepting a file from a stranger and hosting one for them. Inspecting uploads is a losing game — a polyglot is a valid GIF and valid JavaScript at the same time, and every content-type check has a bypass. Decoding to pixels and writing the image out again ends the argument: an appended script tag, an EXIF payload, a trailing archive and an embedded colour profile all cease to exist. A test proves exactly that, on a real crafted polyglot. Also: `[0]` so a multi-frame GIF can't write N files, `-strip` so a screenshot's GPS coordinates don't travel with a bug report, and a max-edge cap because a 30000x30000 PNG is a decompression bomb whatever its byte size. Fails CLOSED. Anything it cannot read as an image is refused whatever the filename claimed, and a missing ImageMagick binary returns an error rather than raising or — the dangerous option — quietly copying the original through. A test pins that, because the value of this function is entirely in what it refuses.
The upload component wrapped itself in a <form> unconditionally, so any host embedding it got nested forms — invalid HTML that the browser resolves by closing the OUTER form at the inner tag. Every field after that point, including the host's submit button and its anti-spam field, silently fell outside the form. Nothing looked wrong. A `standalone` attr (default true, so existing callers are unchanged) now omits the wrapper, and the drop zone is extracted so both paths share it. sanitize/3 read the image dimensions and used them only for a log line. -resize bounds the OUTPUT; the decoder still rasterizes the input in full first, so a small PNG declaring 50000x50000 is ~10GB of RAM before a pixel is written. The -limit flags turn that into a failure rather than an outage; refusing on the header costs nothing and never starts it. Capped at 40MP.
file_uuids is a JSONB array rather than an FK column, so the reference is invisible to the joins orphaned_files_query does — and what it cannot see, it hands to DeleteOrphanedFileJob. Every published board image looked unreferenced. Nothing sweeps today, so this was latent rather than active, which is precisely the kind that surfaces as unexplained data loss later. Verified end-to-end in the projects suite rather than by inspection: a hand-written containment fragment over JSONB is exactly the sort that silently matches nothing (or everything), and both directions are asserted.
`mentions_redact_titles` is off by default, and that is the right call for internal readers — a record's name is rarely the secret. It says nothing about the open web, and both audiences share one renderer. So a public page can now ask for withholding on its own account: a mention outside what the page may show resolves to `:private`, with no title AND no fallback to the author's stored label (the label is the record's name as of writing, so falling back to it publishes exactly what withholding was for). Two holes found by an external panel and confirmed here: `to_markdown/2` had no `:private` clause, so the catch-all emitted `token.label` — passing the flag into a markdown render was a silent no-op, and comment bodies sit right next to a description that withheld properly. `user` is a public type, so a user mention is always allowed, and its resolved path is `/admin/users/view/:uuid`. Linking that from a public page hands an anonymous reader an admin route and a user uuid, and quietly reads "listed inside the admin app" as "public on the open web". On a withheld surface a person is a name, not a door.
`CommentResources` is the registered ResourceLinks handler for the "user"
type, so it names every @ mention there is — public issue boards included —
and it ended `|| user.email`. A commenter who had never set a name had
their full address rendered next to their words on an indexable page.
Proven before the fix:
%{state: :ok, title: "nameless-5954@example.com"}
`User.display_name/1` is now the single answer: organisation name, then
first+last, then username, then the local part of the email, then "User".
Never the address. Every rung is trimmed and rejected when blank, because
`full_name/1` returns "" (not nil) for a first name of " " and an
unguarded chain stops dead on it.
Only fields the USER wrote about themselves feed it. A name recorded by
somebody else — the staff module's HR record, a CRM contact label — must
never reach it. The people with no name set are precisely the ones who are
pseudonymous by choice or inertia, and publishing an HR-entered legal name
on their behalf is not ours to do. That, rather than a default-on setting,
is what dissolves the consent question.
Collapsed into it: Mentions.Users, CommentResources, media_browser's
creator chip. The mention typeahead also stopped shipping the full address
as a result subtitle, which gave back exactly what the title was protecting.
The local part is not anonymity — john.smith@ still reads as john.smith —
but it carries no domain and is not mailable. Most accounts have an
auto-generated username anyway, so that rung usually wins.
V164 freezes comment attribution: what the reader was shown, and whether
the author was speaking for a project. Resolving either at render rewrites
history — someone leaves or fills in a profile and every comment they made
is silently re-signed. Existing rows stay NULL and resolve live rather than
being backfilled with a history we do not have.
This was referenced Aug 9, 2026
Merged
This was referenced Aug 9, 2026
This was referenced Aug 9, 2026
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Post-merge review of #692 (cross-module mentions, canonical display name, no published email addresses). The subsystem's security thinking is sound — visibility fails closed, titles re-resolve per viewer, notify refuses a preview it can't prove is readable. Five defects in the details. - V165's up/1 stamped the schema version as '163'. Every other migration in the chain stamps its own number, so an install migrated to exactly 165 reported 163 — behind where it started — and the next ensure_current/2 re-ran V164 and V165. A full run to head hid it because V166 stamps immediately after. Leftovers from the V163/V164 draft numbering also fixed: V166's moduledoc reference and V165's dead `schema` binding. - escape_md/1 left `]` unescaped on the reasoning that the token grammar refuses it. True of the author's stored label; false of the live resolved title, which is the record's current name. Renaming a record to `Evil](https://evil.example)` turned every markdown-rendered mention of it into a link to the attacker's URL. Verified both the break and the fix against MDEx, the renderer this project actually uses. - MentionText's moduledoc told hosts to `use PhoenixKit.Mentions.RequestAccess`, which does not exist — the module is PhoenixKit.Mentions.Live, and it needs <.access_request_dialog> rendered or the click sets an assign nothing reads. - ImageProcessor.sanitize/3 has no callers; documented as a primitive rather than protection now in place. Reported and deliberately not applied: the @ typeahead runs ~130 queries per keystroke (Scope.for_user/1 per candidate, over-fetching 64), and the fix is an authorization filter rewritten as SQL — not a change to make without executing the suite. Also unfixed: context/2's batching is unavailable to <.mention_text>, which calls it per component. Review: dev_docs/pull_requests/2026/692-add-mentions-display-name-no-email/CLAUDE_REVIEW.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mdon added a commit
to mdon/phoenix_kit_ecommerce
that referenced
this pull request
Aug 9, 2026
PhoenixKitEcommerce.Slugify existed because the rule had drifted between Product
and Category twice — Cyrillic, then German — and each fix reached only one of
them. It is deleted: there is now one implementation for the whole ecosystem, in
the locale_slug package, reached through PhoenixKit.Utils.Slug. A third drift is
not reachable because there is no second copy to drift from.
The bigger fix is that both schemas were DISCARDING the language:
Enum.reduce(title_map, slug_map, fn {lang, title}, acc ->
generated = slugify(title) # lang bound, unused
so every language got the neutral rule. Slug generation is locale-dependent —
German ö expands to oe, Estonian ö folds to o — and passing `lang` is the whole
point of a per-language slug map. AITranslatable had the same shape: it had
target_lang in scope and called Product.slugify/1 without it.
BEHAVIOUR CHANGE worth reviewing: the old module expanded German umlauts
UNCONDITIONALLY, for every language. That was the same bug the `slugger` package
has — correct for German, wrong for Estonian. Expansion is now conditional on
the language:
"Größe", lang "de" -> "groesse" (unchanged)
"Größe", lang "et" -> "grosse" (was "groesse" — wrong before)
"Größe", no lang -> "grosse" (was "groesse")
Stored slugs are NOT rewritten — the changesets only generate when the slug is
blank — so live URLs are untouched. Only newly generated slugs differ, and both
schemas carry a unique constraint for the collision case.
Tests updated to pin the per-language contract rather than the old
expand-for-everyone behaviour.
Requires the phoenix_kit release carrying the locale-aware Slug
(BeamLabEU/phoenix_kit#692). Verified against local core: 454 tests, 0 failures.mdon added a commit
to mdon/phoenix_kit_posts
that referenced
this pull request
Aug 9, 2026
Five sites (post, post_tag, post_group schemas + the post and group edit
LiveViews) each had their own:
|> String.downcase()
|> String.replace(~r/[^\w\s-]/, "")
|> String.replace(~r/\s+/, "-")
|> String.trim("-")
That deletes every non-ASCII character, so a Cyrillic or Greek title produced
an EMPTY slug and German lost its umlauts and ß:
"Видеопродакшн" -> "" now "videoprodakshn"
"Καλημέρα" -> "" now "kalimera"
"Größe Fußball" -> "gre-fuball" now "grosse-fussball"
An empty slug is worse than a wrong one: callers read it as "no slug yet" and
regenerate on every save.
Now delegates to PhoenixKit.Utils.Slug, which is backed by the locale_slug
package — one implementation for the whole ecosystem rather than five copies
that can drift apart.
Stored slugs are NOT rewritten; the changesets only generate when the slug is
blank, so existing URLs are untouched.
Requires the phoenix_kit release carrying the locale-aware Slug (currently in
BeamLabEU/phoenix_kit#692). Verified locally against that core: 22 tests, 0
failures.mdon added a commit
to mdon/phoenix_kit_newsletters
that referenced
this pull request
Aug 9, 2026
list.ex stripped every non-ASCII character, so a Cyrillic or Greek list name produced an EMPTY slug. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug). Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692).
mdon added a commit
to mdon/phoenix_kit_publishing
that referenced
this pull request
Aug 9, 2026
publishing_group.ex derived its slug with an ASCII-only pipeline, so a Cyrillic or Greek group name produced an EMPTY slug. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug). SlugHelpers already delegated to core, so this closes the one place that did not. Stored slugs are not rewritten. Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692). Verified against local core: 1563 tests, 0 failures.
mdon added a commit
to mdon/phoenix_kit_dashboards
that referenced
this pull request
Aug 9, 2026
dashboard.ex stripped every non-ASCII character, so a Cyrillic or Greek dashboard title fell through to the "dashboard" fallback instead of getting a real slug. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug); the "dashboard" fallback is kept for genuinely content-less titles. Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692). Verified against local core: 239 tests, 2 failures, both PRE-EXISTING and unrelated (widget grid placement / legacy string spans in dashboards_test.exs — same 2 fail with this change stashed).
mdon added a commit
to mdon/phoenix_kit_document_creator
that referenced
this pull request
Aug 9, 2026
template.ex stripped every non-ASCII character, so a Cyrillic or Greek template name produced an EMPTY slug. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug). Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692).
mdon added a commit
to mdon/phoenix_kit_projects
that referenced
this pull request
Aug 9, 2026
project_status.ex stripped every non-ASCII character, so a Cyrillic or Greek status name produced an EMPTY slug — and statuses are looked up BY slug (Statuses.current_status/1 treats "" as nil), so the status silently vanished. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug). Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692).⚠️ This repo does not currently compile against local core for a reason unrelated to this change: project_form_live.ex and template_form_live.ex call `ai_multilang_tabs/1`, which needs a newer phoenix_kit_ai than is resolved. The same failure occurs with this change stashed.
This was referenced Aug 9, 2026
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Prepared, NOT published: `mix phoenix_kit.release_check` blocks on a stale ExpectedSchema manifest. See the release note in the #692 review — V165 adds two tables and V166 four columns, and none of them are declared in ExpectedSchema.objects/1, so restamping chain_hash is the wrong tool by that script's own rules. Regenerating needs a pre-squash checkout and a database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
#692 added V165 and V166 without regenerating ExpectedSchema, so chain_hash is stale and release_check refuses to publish. Recorded with the reason the DB-free restamp script is the wrong tool here — V165 creates two tables and V166 adds four columns, none of which the manifest declares — and the two ways forward. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
dev_docs/squash/README.md says outright that a migration PR may land with a stale manifest and that regeneration is the release-time gate, so filing this as a BUG against #692 was wrong. Reclassified as a release blocker owned by whoever publishes. Also records the gap the phrase "just regenerate" hides: the runbook requires regenerating from a PRE-SQUASH checkout, whose chain ends at V163, so no generator run can ever emit V164/V165/V166 objects — which is why V164's are hand-written DECLARED POST-GENERATION CORRECTIONs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mdon added a commit
to mdon/phoenix_kit_posts
that referenced
this pull request
Aug 9, 2026
Five sites (post, post_tag, post_group schemas + the post and group edit
LiveViews) each had their own:
|> String.downcase()
|> String.replace(~r/[^\w\s-]/, "")
|> String.replace(~r/\s+/, "-")
|> String.trim("-")
That deletes every non-ASCII character, so a Cyrillic or Greek title produced
an EMPTY slug and German lost its umlauts and ß:
"Видеопродакшн" -> "" now "videoprodakshn"
"Καλημέρα" -> "" now "kalimera"
"Größe Fußball" -> "gre-fuball" now "grosse-fussball"
An empty slug is worse than a wrong one: callers read it as "no slug yet" and
regenerate on every save.
Now delegates to PhoenixKit.Utils.Slug, which is backed by the locale_slug
package — one implementation for the whole ecosystem rather than five copies
that can drift apart.
Stored slugs are NOT rewritten; the changesets only generate when the slug is
blank, so existing URLs are untouched.
Requires the phoenix_kit release carrying the locale-aware Slug (currently in
BeamLabEU/phoenix_kit#692). Verified locally against that core: 22 tests, 0
failures.mdon added a commit
to mdon/phoenix_kit_projects
that referenced
this pull request
Aug 9, 2026
project_status.ex stripped every non-ASCII character, so a Cyrillic or Greek status name produced an EMPTY slug — and statuses are looked up BY slug (Statuses.current_status/1 treats "" as nil), so the status silently vanished. Now delegates to PhoenixKit.Utils.Slug (backed by locale_slug). Requires the phoenix_kit release carrying the locale-aware Slug (BeamLabEU/phoenix_kit#692).⚠️ This repo does not currently compile against local core for a reason unrelated to this change: project_form_live.ex and template_form_live.ex call `ai_multilang_tabs/1`, which needs a newer phoenix_kit_ai than is resolved. The same failure occurs with this change stashed.
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Not my work — Pincer's pre-merge surface review of #692, left untracked in the working tree. Committing it alongside CLAUDE_REVIEW.md in the same directory so both halves of the review are on record, and so the release gate's clean-tree check passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Post-merge review of #694. The headline fix is real and the PR self-corrected two significant defects before merge (the first commit fixed the wrong twin; the new atom crashed status through a clause the author had not extended) — both documented plainly in c661f1b. Two gaps left, one of them red on main. - StatusReport's `action()` type never gained `{:fix_version_comment, String.t()}`. next_action/3 is @SPEC'd to return `action()`, so dialyzer proved the new `format_next_action({:fix_version_comment, _})` clause unreachable and exited 2. `mix precommit` therefore fails on main as merged; every later PR inherits a red gate. The PR's stated verification covers compile, format and credo — three of the four steps precommit runs, and dialyzer is the one that catches this. - The @doc return lists for check_installation_status/1 and check_update_needed/2 both still enumerated the old states. That list is the contract callers read to decide which clauses to write, and this PR already paid for it being wrong once. Verified rather than assumed: all three consumers do handle the new state; repair_uuid does check castability, so V163's reordered deferral is not a dead end; the three version-comment parsers agree except on non-positive integers, where the new one is the stricter; 86 tests + 6 doctests pass. The mentions/users.ex dialyzer ignore #692 added is load-bearing — removing it goes red. Note for the release: #694's v163/v164 edits are guard-ordering and detection shape only, so they stay restamp-safe. V165/V166 remain the only versions that add manifest objects. Review: dev_docs/pull_requests/2026/694-stop-reporting-comment-less-database-as-version-1/CLAUDE_REVIEW.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
I created a second directory for #694 without checking for an existing one. Moved CLAUDE_REVIEW.md alongside Pincer's phase1.md, which was there first, matching how #692's two halves sit together. phase1.md is not my work — committing it so both halves are on record and the tree stays clean for the release gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Both clean — no new defects. Verified the parts this series has repeatedly got wrong rather than the parts the diffs touch: #695 introduces two new return values and both propagate completely. estimated_rows/3's :unknown is handled by an explicit pattern match in V163 (not by term ordering, which would have made it correct by accident) and named out loud by repair_uuid. Probe's :unparseable reaches classify/3, Repair.run, error_message/1, the error type, and the JSON error_tag — traced each. The '0' behaviour change it implies is unreachable: nothing stamps IS '0'. #696's "two siblings" claim is exhaustive: no `_system ->` left in auth.ex, and delete_user was already fail-closed by a different mechanism (validate_can_delete_user/2's fallback refuses even nil). It also corrects a test I added in the #692 pass, and the correction is worse than its commit message claims. My version poisoned all five identity fields in one payload; "attacker-user_timezone" fails validate_user_timezone/1, one invalid member invalidates the whole profile_changeset, so nothing was written either way. It passed with the filter DELETED and failed with the filter working — inverted, not merely vacuous, and red on main since I added it. The exact defect I spent three reviews naming in other people's tests, in a test I could not run. Written up in full in the #696 review. CHANGELOG deliberately NOT committed: another agent is mid-edit in this tree (implementing the mentions N+1 and access-request findings from my #692 review, and hand-declaring V165/V166 in the manifest), and CHANGELOG.md now holds their in-flight entries alongside mine. Committing it would separate their notes from their code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 10, 2026
…6 manifest Not my work — another agent's changes, uncommitted in the working tree while I was reviewing #695/#696/#697. Committing at the maintainer's request so nothing is stranded. Verified it compiles with warnings-as-errors before committing. Three pieces, all closing findings from my earlier review passes: - Mentions.Users.search/2 no longer calls Scope.for_user/1 per candidate; the admin-area rule became two SQL EXISTS subqueries with the limit applied in the database. This is the ~128-queries-per-keystroke item I reported in the #692 review and deliberately did not fix, because rewriting an authorization filter as hand-written SQL without a database to test against was not a change to make blind. It has a database now. - AccessRequests.request/4 validates the resource type against ResourceLinks.handlers/0, requires a castable uuid, and rate-limits per account — the unvalidated/unthrottled path I filed as a nitpick on the same PR. - ExpectedSchema hand-declares V165/V166 objects and restamps chain_hash, which is the release blocker. Their choice of the hand-declare route matches what the #692 review concluded was necessary: no generator run can emit V164+ objects, because regeneration replays a pre-squash checkout whose chain ends at V163. CHANGELOG.md is deliberately NOT in this commit. The copy in this tree predates the 2.0.0 retitle and the #695-#697 entries now on main, so committing it would have reverted them. Their three entries were extracted verbatim first and are re-applied on top of the current file in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 10, 2026
Two loose ends from committing the other agent's work. The chain_hash they wrote was computed before #695 landed, and #695 edited v163.ex (cond branch order) and v164.ex (a detection query's contype filter, a wrapped probe). So the stamp was behind by exactly those two edits while the manifest BODY — their hand-declared V165/V166 objects — was already correct. Restamped over the 32 shipped files, which is legitimate here for the reason the script requires: both #695 edits are guard, probe and message logic and add no schema object, established in the #695 review. Recorded inline, including what a restamp still does not assert. release_check is now 5/6 and the two unit tests that had been red on main since #692 pass again (test/mix/tasks/phoenix_kit_release_check_test.exs, 15 tests). CHANGELOG: their three recheck entries re-applied on top of the 2.0.0 file, since the copy in this tree predated the retitle and would have reverted it.⚠️ Still outstanding, and not something a green gate should be read as covering: the V165/V166 objects in the manifest have never been checked against a real database. `verify.exs --scenario s7,s8` is what would do that, and #689's equivalence evidence still predates HEAD. Both need the PostgreSQL that has not been reachable in any of these passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Cross-module
@/#mentions, one canonical display name, and the plumbing twomodules needed to stop leaking email addresses onto public pages.
Rebased onto
upstream/maintoday. Both of this branch's migrations wererenumbered during that rebase — see "Migration chain" below.
What's here
Mentions (V165) —
@[user:uuid|Label]/#[type:uuid|Label]tokens thatsurvive markdown, sanitisation and copy-paste, a typeahead any textarea can
opt into (
use PhoenixKit.Mentions.Live), a reverse index for "what linkshere", and access requests for a record the reader can see named but not open.
Visibility is federated to whichever module owns the type and fails closed:
a type that declares no check resolves to nothing rather than everything.
User.display_name/1— one answer to "what do we call this person infront of other people": organisation name → first+last → username → the local
part of the email →
"User". Never the full address.This replaced four private copies, two of which ended
|| user.email. One ofthem,
Users.CommentResources, is the registeredResourceLinkshandler forthe
"user"type — so it names every@mention there is, and a publicissue board was rendering commenters' full addresses next to their words:
Only fields the user wrote about themselves feed it. A name recorded by
somebody else — an HR record, a CRM label — never reaches it.
withhold_titles(V166 adjacent) —mentions_redact_titlesis off bydefault, which is the right call for internal readers and says nothing about
the open web. Both audiences share one renderer, so a public surface can now
withhold on its own account: an out-of-scope mention renders with no title
and no fallback to the author's stored label (the label is the record's name
as of writing — the same leak, one revision staler).
Frozen comment attribution (V166) —
author_display_name,attribution_mode,attributed_project_uuid,attributed_labelonphoenix_kit_comments. A name resolved at render time rewrites history:someone leaves or fills in a profile and every comment they ever wrote is
silently re-signed.
user_uuidis never cleared — posting under a project'sname changes what the public sees and nothing else, so moderation and audit
keep their actor.
Upload sanitiser —
ImageProcessor.sanitize/3re-encodes rather thanre-serving: coder forced from the sniffed type,
-strip, resource-limits,and a 40 MP cap before
convertis invoked (-resizebounds the output; thedecoder still rasterises the input in full, so a 5 MB PNG declaring
50000×50000 is ~10 GB before a pixel is written).
Smaller fixes —
file_uploadwrapped itself in a<form>unconditionally,so any host embedding it got nested forms and the browser silently moved every
field after that point out of the outer form. Orphan detection learned about
JSONB-referenced attachments, which it would otherwise have collected as
unreferenced while they were live on a public page.
Migration chain
This branch originally used V163 and V164. Upstream claimed both while
it was local (UUID primary-key integrity; the V56/V57 flush-order repair), so
the rebase renumbered them to V165 (mentions) and V166 (attribution),
including their
down/1markers.release_checkconfirms the result:ExpectedSchema.chain_hash/0no longer matches, so these two fail:check_manifest_chain_hash/0check_migration_sync/0(only on the hash assertion — floor, contiguity andloadability all pass, as above)
Per
dev_docs/squash/README.md, this is expected and not something to fixin this PR:
Regeneration has to run from a pre-squash checkout (on the squashed branch
the generator is self-referential and drops pre-floor drift), and
restamp_chain_hash.exsis explicitly the wrong tool here — it would make thehash pass over a manifest that has no knowledge of these objects, hiding the
drift the hash exists to detect. Hand-merging the manifest is forbidden by the
same policy. So: regenerate before publishing, please.
Tests
3259 tests, 2 failures— the two above.Run with
--max-cases 4. At full concurrency the integration suite producesdeadlock_detectedfailures in a different handful of tests each run; the samehappens on a clean
upstream/mainworktree, so it is pre-existing andunrelated to this branch.
mix precommitexits 0.