Uh oh!
There was an error while loading. Please reload this page.
Add UrlState: URL-backed search, filter, sort and page for list LiveViews - #680
Conversation
…iews Convert the users admin list to it and record the router-only contract on MediaBrowser.Embed, which has always had it implicitly.
Sessions, live sessions, jobs and the media selector now carry search,
filters, sort and page in the query string. Live sessions gains URL-backed
sort by splitting the compound %{by:, dir:} assign into two flat params and
recombining them, so its template is untouched.
url_state_path/2 now also accepts a bare assigns map: LiveView swaps
socket.assigns for %AssignsNotInSocket{} while rendering, so @socket cannot
carry state into a template.- Cap integer params at 1_000_000 by default. An unbounded ?page= survives Integer.parse, reaches Ecto as OFFSET and overflows PostgreSQL's bigint, turning a crafted link into a 500 on every converted list. - Raise on a change keyed by URL key rather than assign name. It previously did nothing but reset the page, presenting as "search is broken" with no error anywhere. - Make reload?/3 public so the reload decision is testable without a router. - Correct the dead_render: :skip docs: the callback not running means its assigns do not exist, so the dead render raises rather than painting empty.
timujinne
commented
Aug 4, 2026
Review roundsTwo Round 1 — APPROVE with findings. Three fixed in
Accepted gap: LiveView-level tests of the Round 2 — one BUG-HIGH reported, which does not reproduce. The reviewer read Everything else in round 2 verified clean: identical-URL pushes cannot leave a screen stale (media_selector reloads in place when already on page 1; the other four route mutations through a direct |
normalize!/2 ran at macro expansion, where option values are still quoted.
A literal `in: [:asc, :desc]` is a list of atoms in AST too and worked by
accident, but `in: ~w(name email)` is a {:sigil_w, ...} node — matching a URL
value against it raised Protocol.UndefinedError at request time, past
compilation and past the codec tests. Normalise in the caller's module body
instead, and pin it with a test that actually uses the macro.url_state_path/2 encoded whatever a caller merged in. A screen re-picking a sort column after the current one is hidden hands over whatever column is left, sortable or not — the URL then named a value decode/2 refuses, so the address bar and the assigns disagreed and a reload showed something else. Sanitise the merged state against the same whitelist and bounds before encoding.
timujinne
commented
Aug 4, 2026
Third finding, from reviewing the first consumerReviewing the warehouse conversion (BeamLabEU/phoenix_kit_warehouse#11) surfaced a gap in this module, fixed here in
It is not hypothetical. Warehouse's list screens re-pick the sort column when the current one is hidden, handing over whatever column is left — sortable or not. An unsortable pick was written to The merged state is now sanitised against the same whitelist and bounds as decoding, so the invariant is symmetric: the URL never carries a value the decoder would reject. Covered by a test. That review also found two defects in warehouse itself (a sort reset bypassing the URL, and a lost query cache); both are fixed on that PR. Worth noting the shape of it — the module's own tests were green throughout, and both this gap and the earlier |
url_state_path/2 merged onto the bookkeeping state map, so a LiveView that set a declared param with assign/3 saw the superseded value come back in the URL on the next patch — and a reload apply it. Warehouse hit this in all seven lists: re-picking the sort column after the active one was hidden left ?sort= naming the hidden column. Read the merge base back from the individual assigns instead, falling back to the stored map. The freshest value wins however it was set, so the failure mode is a URL that catches up rather than one that lies.
push_patch requires handle_params/3 to be exported, and exporting it is exactly what makes a LiveView impossible to embed with live_render/3. One requires what the other forbids, so until now an embeddable list simply could not carry its state in the URL — phoenix_kit_projects has ten such LiveViews, and more modules are heading the same way. mode: :history never touches handle_params at all. The :handle_params stage raises outright when the view has no router, so the hook goes on :handle_event instead, and the compile-time stub is not injected. The browser owns the URL: a JS hook reports the query on connect, rewrites the address bar when the server pushes a new one, and reports popstate — which is what makes Back work without a router. Only the query crosses the wire; the path stays client-side, because an embedded LiveView does not know what page it is on. The first load stays in mount/3, since there is no handle_params to hang it on, so handle_url_state/2 serves changes only. The state is marked loaded at mount so the client's connect report costs nothing when the URL held nothing.
timujinne
commented
Aug 5, 2026
Added: |
Reviews the UrlState / V161-citext / V162-payment-option wave merged on
main, and fixes what it turned up.
Fix: `get_user_by_email_or_username_and_password/3` hand-rolled its case
folding as `fragment("LOWER(?)", u.username)`, which matches no index in
the chain. V161's whole premise is that comparison semantics come from
the column type, so with `username` now `citext` plain equality is both
correct and index-backed via `phoenix_kit_users_username_uidx`. This was
the only username lookup still sequentially scanning the users table, on
the one endpoint reachable without authenticating.
Fix: the `PhoenixKitUrlState` JS hook registered `handleEvent` on the
LiveSocket but only removed its `popstate` listener in `destroyed()`, so
each remount left another live callback behind.
Add: `test/phoenix_kit/migrations/v162_test.exs`. V162 shipped with no
test. Pins `ON DELETE SET NULL` in particular — that is the migration's
whole design decision, and a later refactor reaching for a plain
`references/2` would silently make it `RESTRICT` with nothing failing.
Also renames the V162 PR-draft doc out of `680-v161-…`, which named the
pre-renumber identity and collided with PR #680's own directory.
Full findings, and the two recorded-but-unfixed gaps, in
dev_docs/pull_requests/2026/680-682-post-merge-review/CLAUDE_REVIEW.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The GLM reviewer pass on PR BeamLabEU#675 (COALESCE guard for the atomic custom_fields merge/delete) was left untracked in a second, branch-name mismatched directory. Filed under the PR's existing directory, whose slug matches the head branch fix-custom-fields-atomic-merge, per the one-directory-per-PR convention in CLAUDE.md. That puts it next to the PR's CLAUDE_REVIEW.md; the two earlier GLM reviews (BeamLabEU#668, BeamLabEU#680) instead sit in their own slug directories, which is drift worth not repeating. .pi-subagents/ holds mission JSON and run transcripts written by the external subagent harness; its reports land outside the repo, so the directory is scratch and is now ignored rather than committed.
Why
Typing in a list's search box filters the table but leaves the address bar untouched on most admin screens. The result cannot be shared, does not survive a reload, and Back walks out of the page instead of back to the previous query.
A workspace-wide audit found 26 LiveViews with this defect and — more to the point — seven independently hand-rolled implementations of the fix on the screens that do work:
MediaBrowser.Embed,Activity.Index, and one each incomments,billing,crm,emailsandecommerce. They differ only cosmetically (maybe_putvsEnum.reject, flat vs nested form params).Six of the seven share a real defect: they rebuild the path from a literal (
Routes.path("/admin/comments?…")), so a LiveView reachable at more than one route — a sub-tab such as/orders/:id/edit/files— patches itself to the wrong page.What this adds
PhoenixKitWeb.Live.UrlState— declare the state, implement one callback, push changes:url_key:naming the query key separately — so conversions touch no templates.users.html.heex(700 lines) needed no edit.alias:is read but never written, letting screens that already published?search=links converge on?q=without breaking them.cast:/in:whitelist values; a forged one falls back to the default. No atom is ever created from user input./admin/users, not/admin/users?q=&role=all&page=1.uri, not a literal — locale segments and parent-resource ids survive.replace: truefor debounced input: a typed-out query leaves one history entry instead of one per pause. Every existing implementation gets this wrong, so Back currently walks the search box backwards a few characters at a time.?return_to=…&mode=singlesurvives a search.Converted
users,sessions,live_sessions,jobs/index,media_selector.live_sessionsalso gains URL-backed sort: its compound%{by:, dir:}assign is split into two flat params and recombined in the callback, leaving the template untouched.use UrlStatemakes a LiveView router-onlyVerified against
phoenix_live_viewsource, and the reasonphoenix_kit_projectsis out of scope:push_patchreachessync_handle_params_with_live_redirect/5, which callsUtils.call_handle_params!/4— the arity whoseexported?defaults totrue. Sohandle_params/3must be exported.root_pid != self()),maybe_call_mount_handle_params/4seesany? = callbacks? or exported?and raises throughRoute.live_link_info!. So exportinghandle_params/3— whatever its body — makes a LiveView un-embeddable.One requires what the other forbids.
MediaBrowser.Embed'surl_sync: truehas always had this constraint through its injected stub; this PR writes it into its moduledoc.Tests
41 tests, no PostgreSQL required — everything deciding a URL is pure. Covers default-dropping, alias decode, cast/whitelist rejection, the integer ceiling, page reset, unknown-key preservation, path capture and fallbacks, and
reload?/3.Not covered: the
on_mount→ hook → hosthandle_params/3ordering.live_isolated/3mounts without a router, which this module refuses by design, so testing it needs a test router and endpoint core does not have yet. Called out in the design doc rather than papered over.Review
ask-glm(elixir-review) reviewed the branch and approved it with four findings; three are fixed ind186b395:?page=survivesInteger.parse, reaches Ecto asOFFSETand overflows PostgreSQL'sbigint, turning a crafted link into a 500;push_url_statenow raises on a change keyed by URL key instead of assign name, which previously did nothing but reset the page;dead_render: :skipdocs were wrong — the callback not running means its assigns do not exist, so the dead render raises rather than painting empty.The fourth (LiveView-level test coverage) is the gap recorded above.
Design doc
dev_docs/plans/2026-08-04-url-state-search-persistence.md— full audit table, the constraint derivation, and the rollout order for the remaining repos.