Uh oh!
There was an error while loading. Please reload this page.
PR #552 follow-ups: dedup + 3 missed sites + 22 new tests - #554
Merged
Conversation
…fix` pass Three follow-up fixes surfaced by browser testing + an ast-grep sweep of all URL-emission sites: ## `process_valid_locale/2` in users/auth.ex The post-PR BeamLabEU#551 plug unconditionally 301-redirected `/<default>/...` → `/...` for non-admin paths. With the new setting OFF (the default — `/<default>/...` is canonical), the 301 was firing on every primary-language request **including form POSTs**, which silently discarded the POST body. Browser test caught this: login was completely broken with the default setting. Fixed by gating the redirect on `prefixless_primary?/0`. With setting OFF the prefixed shape is canonical and never gets stripped; with ON the strip restores the canonical-redirect behavior from PR BeamLabEU#551. ## Sitemap sources `static.ex` + `posts.ex` Both had the same bug as the publishing sitemap source I fixed in the original commit — `build_path_with_language/3` accepted an `_is_default` parameter but ignored it, emitting the prefix for the primary language in multilang mode regardless of the setting. Found by `ast-grep --lang elixir --pattern '"/#{$LOCALE}#{$PATH}"'`. Both follow the publishing source's pattern: `cond` block with explicit skip rules (nil language, single-language mode, primary + setting-on), prefix in all other cases. ## Browser verification Toggled the setting on the Languages admin page, regenerated the sitemap, and confirmed: - Setting OFF → primary entries get `/en/`, non-primary get their own prefix - Setting ON → primary entries drop `/en/`, non-primary unchanged `/en/db-test-1` 301-redirects to `/db-test-1` when setting is ON (canonical redirect intact); the same URL serves directly when setting is OFF (no redirect, POST bodies safe). ## Sweep coverage ast-grep + grep across all 14 phoenix_kit-* repos for hardcoded `/en/` segments, manual locale concatenation, raw `redirect(to:)`, hreflang/canonical emission, `~p` verified routes, og:url, and RSS/Atom feeds. No additional misses — every other URL builder either uses `Routes.path/1` (now gated) or the publishing / entities helpers (already delegated to the core setting). `mix test`: 1308 tests, 0 failures, 11 doctests. `mix compile --warnings-as-errors`, `mix credo --strict`, `mix format --check-formatted`, `mix deps.unlock --check-unused` all clean.
Surfaced by a deeper ast-grep + grep sweep across all repos for locale/language URL-emission patterns. Every other site I checked already routes through `Routes.path/admin_path` (gated) or the publishing/entities helpers (delegated). One outlier: `redirect_invalid_locale/2` (`lib/phoenix_kit_web/users/auth.ex`) strips an invalid locale segment from the URL and redirects to the cleaned path. It always emitted the *prefixless* shape, which was correct under PR BeamLabEU#551 (always-strip primary) but inconsistent with the new setting OFF (default) where the canonical primary shape is prefixed. Example: `/phoenix_kit/xx/admin/users` (xx = invalid locale) was redirecting to `/phoenix_kit/admin/users` regardless of setting. With setting OFF, the rest of the app emits `/phoenix_kit/en/admin/users`, so the redirect was landing on a non-canonical shape. Both shapes resolve at the router (dual-scope admin emission), but the inconsistency means an external link with an invalid locale lands users on a path that doesn't match the canonical URL emitted everywhere else in the app. Fixed by gating the replacement segment on `prefixless_primary?/0`: - Setting ON → strip entirely (canonical primary is prefixless) - Setting OFF → swap the invalid segment for `/<default_base>/` (canonical primary is prefixed) ## Sweep coverage (no additional misses) - All `def *path*` / `def *url*` / `def *href*` builders that take a locale param — verified to route through gated helpers. - All `String.replace(..., "/#{locale}/", ...)` sites — only used in invalid-locale stripping (above) and the now-gated canonical redirect. - All `~p` verified routes — static assets only, no locale. - All `og:url` / canonical / hreflang emitters — use `PublishingHTML.build_post_url` or `UrlResolver.public_url`, both gated. - All `<.link navigate="/admin/...">` and `<.pk_link>` — pk_link routes through `Routes.path/1`; the one bare `<.link>` hit was inside a moduledoc example, not runtime code. - Language-switcher dropdown's `resolve_url/3` → `generate_base_code_url/2` — uses `Routes.admin_path` and `Routes.path`, both gated. No remaining misses across phoenix_kit, publishing, entities, projects, ai, catalogue, newsletters, staff, comments, user_connections, legal, sync, emails, posts, locations, or hello_world. `mix test`: 1308 tests, 0 failures, 11 doctests. `mix compile --warnings-as-errors`, `mix credo --strict`, `mix format --check-formatted`, `mix deps.unlock --check-unused` all clean.
…wrapper + tests Phase 2 triage on PR BeamLabEU#552 surfaced three actionable items: ## Dedup `build_path_with_language/3` across 3 sitemap sources The site-wide `default_language_no_prefix` setting gate logic was byte-identical across `publishing.ex`, `static.ex`, and `posts.ex` sitemap sources, alongside three byte-identical `single_language_mode?/0` private helpers. Extracted to a new `PhoenixKit.Modules.Sitemap.LocalePath` module: - `emit_prefix?/2` — single canonical decision rule - `single_language_mode?/0` — shared defensive lookup Each source still owns its locale-segment **formatting** (publishing uses `get_display_code/2` for hreflang-aware dialect emission; static + posts use `DialectMapper.extract_base/1` for clean base codes), so the dedup is decision-only. Three sources lose ~30 LOC each. ## Dedup `prefixless_primary?/0` boot-safe wrapper `PhoenixKit.Utils.Routes` and `PhoenixKitWeb.Users.Auth` each had a private `prefixless_primary?/0` wrapper around `Languages.default_language_no_prefix?/0` with defensive rescue — the Routes one also guarded against mix-task context, the Auth one didn't. Promoted to a public `Languages.prefixless_primary_safe?/0` with both guards (mix-task context check + broad rescue → false). Both callers now delegate to the canonical implementation. ## New test coverage The original integration test covered setter round-trip, Routes helpers, and `migrate_legacy/0`. Filled the gaps surfaced by triage: - `test/integration/sitemap/locale_path_test.exs` (NEW, 8 tests) — the shared `LocalePath.emit_prefix?/2` policy across single- language, multi-language, setting OFF, setting ON, primary, non-primary, and `is_default` mismatch cases. - `test/integration/users/auth_locale_test.exs` (NEW, 4 tests) — `redirect_invalid_locale/2` swap-vs-strip behaviour gated on the setting. Was previously untested; the gating only landed in this PR (commit `41a70b36`) and would have regressed undetected. - `default_language_no_prefix_test.exs` adds 2 tests for the new `prefixless_primary_safe?/0` wrapper covering both the runtime path and the mix-task-context fallback. ## Triage findings classified NOT-A-FIX - **Three-repo `prefixless_primary?/0` triplication** (core, auth, entities) — each repo has a distinct rescue scope and boot timing. Entities can't depend on core's mix-task-context sentinel because it doesn't run during core's install path. Leaving the entities copy as-is is the right call; the dedup target was the two **in core** which now share `prefixless_primary_safe?/0`. - **`default_language_no_prefix_key/0` public exposure** — flagged by triage as "looks like internal API". It's intentionally public for the legacy-migration system; the `@doc` says so. No change. - **Async UX on the Languages toggle** — single phx-click on a checkbox; round-trip is a fast Settings write + ETS-cache invalidate. No `phx-disable-with` needed. ## Verification `mix test`: 1322 tests, 0 failures, 11 doctests. `mix compile --warnings-as-errors`, `mix credo --strict`, `mix deps.unlock --check-unused`, `mix format --check-formatted` all clean.
Two test gaps surfaced when auditing whether every bug we found had test coverage: ## `process_valid_locale/2` canonical-redirect gating The fix in commit `4e096e2f` was the bug that broke login mid- browser-test (the 301-redirect was discarding POST bodies when setting was OFF). The fix gated the redirect on `prefixless_primary?/0` so the `/<default>/...` shape stays canonical when setting is OFF. Added 4 tests in `test/integration/users/auth_locale_test.exs` exercising `validate_and_set_locale/2` (the public entry — the gated logic is in the private `process_valid_locale/2`): - Primary locale on non-admin URL is NOT redirected when setting is OFF (regression coverage for the login bug) - Primary locale on non-admin URL IS redirected when setting is ON - Primary locale on admin URL is NEVER redirected (both settings) — admin dual-scope router emission resolves both shapes - Non-primary locale on non-admin URL is never redirected ## Languages LV `toggle_default_language_no_prefix` event The new toggle handler was the user-facing flow added in commit `911bad36` for flipping the site-wide setting from the admin UI. The behavior was browser-verified but never test-asserted. Added `test/integration/phoenix_kit_web/live/modules/languages_toggle_test.exs` with 4 tests: - Default state renders the toggle as unchecked - Click persists ON and re-renders the input with `checked` - Re-click toggles OFF and re-renders without `checked` - Mount reflects the already-persisted setting The `checked` attribute assertion uses a helper that regex-matches the input element's outerHTML — Phoenix renders multi-line attrs with whitespace that breaks naive substring assertions on `"checked phx-click=\"...\""`. ## NOT covered by the audit retrospective - The sitemap-source `build_path_with_language/3` bug in `static.ex` + `posts.ex` (commit `4e096e2f`) — now covered indirectly by the `LocalePath` policy test in commit `78172ec4`. Both sources delegate trivially to `LocalePath.emit_prefix?/2` so the policy test is the canonical coverage point. `mix test`: 1330 tests, 0 failures, 11 doctests (was 1322, +8 new). `mix compile --warnings-as-errors`, `mix credo --strict`, `mix format --check-formatted`, `mix deps.unlock --check-unused` all clean.
ddon pushed a commit
that referenced
this pull request
May 20, 2026
#554 ships the PR #552 locale-prefix follow-ups (login-redirect gate, LocalePath dedup, redirect_invalid_locale swap-vs-strip, 22 new tests). Verdict: ship. Notes are nitpicks — double prefixless_primary? read in redirect_invalid_locale, moduledoc "three rules" / four-item list, mix_task_context? sentinel duplicated between Languages + Routes. #555 restores the bare-label rendering for single-dialect bases in the language switcher (and inherits it into admin/user dashboard nav via the public dedupe_names/1). Verdict: ship. Notes are nitpicks — unrun doctests on the new helpers, the dedup rule is duplicated between display_name_for/3 and dedupe_one_name/2, extract_base_language_name/1 returns "" on "" input. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
May 20, 2026
Rolls up PR #554 (locale-prefix follow-ups — login regression fix, sitemap source dedup, redirect_invalid_locale swap-vs-strip, 22 new tests), PR #555 (language switcher country-qualifier dedup), and the etcher 0.3 → 0.4 / fresco 0.5.2 → 0.5.3 bumps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
ddon pushed a commit
that referenced
this pull request
May 25, 2026
All three review findings (dead @see ref, doctest evaluation hazard, undefined Setting.t() in @SPEC) fixed pre-existing in post-merge commits 42722ff and 0b6ec6f. The NITPICK about free-floating comments inside cond clauses is N/A — resolved by the dedup PR #554 which moved sitemap-source policy into LocalePath.emit_prefix?/2.
ddon pushed a commit
that referenced
this pull request
May 25, 2026
…er symmetry Three still-live findings from CLAUDE_REVIEW.md: * IMPROVEMENT-MEDIUM — redirect_invalid_locale/2 evaluated prefixless_primary?() twice (once for segment, once for suffix). Single read into a destructured tuple; also closes a small torn-read window if a concurrent setting flip lands between the two calls. * IMPROVEMENT-MEDIUM — LocalePath moduledoc said 'three rules' but listed four. Edited to 'four rules:'. * NITPICK — auth_locale_test.exs cleanup mixed Settings.update_boolean_setting with Languages.set_default_language_no_prefix elsewhere in the same file. Cleanup now uses the typed setter for symmetry; any future cache/invalidation logic the typed setter wires in will run on cleanup too. Two NITPICKs left as-is (intentional residue documented in the FOLLOW_UP): the prefixless_primary_safe?() / mix_task_context?() sentinel duplication (acknowledged in the original review), and the LocalePath.emit_prefix?(nil, _) clause whose rationale lives in moduledoc rather than inline.
ddon pushed a commit
that referenced
this pull request
May 25, 2026
Release rollup since 1.7.120: - PR #568: native <dialog> modal (PkDialog), core list-UI toolkit (BulkSelect, Sortable, ReorderModal, load_more), race-free sort_selector - PR #568 post-merge review fixes (untranslated reorder label, named group/row) - PR #569: PhoenixKit.boot/1 hook, locale-aware Activity dates, broad i18n sweep - PR #550/#552/#554/#557/#558/#559 follow-ups Co-Authored-By: Claude Opus 4.7 (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.
Summary
Four follow-up commits on top of the merged PR #552 that surfaced
from browser testing + a deeper grep/ast-grep sweep + a phase 2
triage pass + a "did we test every bug we fixed?" retrospective.
Rebased onto current
upstream/dev(post boss's docstring + doctestreview fixes from
42722ff2); no conflicts. Boss's CLAUDE_REVIEWartifact in
dev_docs/pull_requests/2026/552-.../CLAUDE_REVIEW.mdis preserved.
The four commits
b757ca01Fix sitemap/auth sites missed by the initial passThree sites the initial PR #552 commit missed, surfaced by browser
testing + ast-grep:
process_valid_locale/2inusers/auth.ex— the post-URL-driven locale resolution + admin-URL prefixless + module_assigns generalization #551plug unconditionally 301-redirected
/<default>/...→/...fornon-admin paths. With the new setting OFF (default —
/<default>/...is canonical), this discarded POST bodies. Login was broken.
Now gated on
prefixless_primary?/0. Browser-caught.sitemap/sources/static.ex+posts.ex— same bug asthe publishing sitemap source (
_is_defaultignored). Found byast-grep --lang elixir --pattern '"/#{$LOCALE}#{$PATH}"'. Bothnow use the same
condblock as publishing.edf63d0fredirect_invalid_locale/2honors the settingThe plug that strips an invalid locale segment (e.g.
/phoenix_kit/xx/admin/users→/phoenix_kit/admin/users) alwaysemitted the prefixless shape. With setting OFF, the canonical
primary shape is prefixed — the redirect was landing users on
a non-canonical URL inconsistent with the rest of the app's
emission. Now swap-vs-strip per setting.
97543a29Phase 2 follow-up: dedup + boot-safe wrapper + testsPhase 2 triage of PR #552:
build_path_with_language/3across 3 sitemap sources(
publishing,static,posts) → new sharedPhoenixKit.Modules.Sitemap.LocalePathmodule owning thedecision (
emit_prefix?/2+single_language_mode?/0). Eachsource keeps its own formatting (publishing uses
get_display_code/2for hreflang-aware dialect emission; staticDialectMapper.extract_base/1for clean basecodes).
prefixless_primary?/0betweenroutes.exandauth.ex(the latter was missing the mix-task-context guard)→ promoted to a public
Languages.prefixless_primary_safe?/0with both guards. Two callers now delegate.
LocalePathpolicy, the auth redirect, andthe boot-safe wrapper.
fac2c665Cover the two test gaps surfaced by the phase 2 auditSelf-audit caught two bugs we fixed without test coverage. Closed:
process_valid_locale/2canonical-redirect gate — 4 testsin
auth_locale_test.exscovering the regression that brokelogin (primary on non-admin with setting OFF must NOT redirect)
redirect, non-primary never redirects).
toggle_default_language_no_prefixhandler —4 tests in
languages_toggle_test.exscovering default state,click persists ON, re-click toggles OFF, mount reflects
persisted state.
Verification
mix test: 1330 tests, 0 failures, 11 doctests (was 1308post-merge, +22 net new).
mix compile --warnings-as-errors,mix credo --strict,mix format --check-formatted,mix deps.unlock --check-unusedall clean.
Browser-verified end-to-end across both setting states: Languages
toggle, admin URL emission, publishing public URLs, sitemap
regeneration, canonical redirects.
Open follow-up from boss's review (not addressed here)
@spec set_default_language_no_prefix/1referencesPhoenixKit.Settings.Setting.t()which has no@type t/0declared on
Setting— dialyzerunknown_typewarning. Bossflagged in
CLAUDE_REVIEW.md; separate fix onSetting.Test plan
/admin/settings/languagesaftermerge → confirms the LV toggle test reflects real behaviour