Add collapsible sidebar and redesign app layout#1748
Draft
Flo0807 wants to merge 138 commits into
Draft
Conversation
Flo0807
marked this pull request as draft
January 8, 2026 11:14
Collaborator
Author
|
As this MR handles persistent state via local storage, it will result in showing the sidebar for a short amount of time on page load. I suggest that we work on a more generic "UI state" solution in a dedicated PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds a collapsible sidebar and redesigns the application layout. The changes replace the previous drawer-based sidebar with a new implementation that supports both mobile overlay and desktop push-aside modes, with state persistence for the desktop view.
Changes:
- Replaced
BackpexSidebarSectionshook with a newBackpexSidebarhook that manages both sidebar visibility and section expansion - Redesigned app shell layout to use a fixed sidebar with overlay on mobile and content-shifting on desktop
- Moved branding component from topbar to sidebar (breaking change from
topbar_brandingtosidebar_branding)
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| assets/js/hooks/_sidebar.js | New hook implementation combining sidebar visibility management with section expansion logic |
| assets/js/hooks/_sidebar_sections.js | Removed old sections-only hook |
| assets/js/hooks/index.js | Updated export to reference new BackpexSidebar hook |
| priv/static/js/backpex.esm.js | Compiled ESM bundle with new sidebar implementation |
| priv/static/js/backpex.cjs.js | Compiled CJS bundle with new sidebar implementation |
| lib/backpex/html/layout.ex | Redesigned app shell layout with fixed sidebar and renamed branding component |
| demo/lib/demo_web/components/layouts/admin.html.heex | Updated demo to use new sidebar structure with sidebar_branding |
| demo/assets/css/app.css | Added CSS custom property for sidebar width |
| priv/gettext/backpex.pot | Updated translation strings for simplified navigation labels |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The priv generator template and installation guide still referenced the removed topbar_branding component and the old sidebar slot shape. Move the branding into the sidebar slot as sidebar_branding and wrap nav content in <nav><ul> so copy-pasted layouts compile and render correctly under the new app_shell API.
Document the breaking sidebar API changes (topbar_branding rename and slot move, required <nav>/<ul> wrapper, --sidebar-width CSS variable, and BackpexSidebarSections hook rename) so existing users can migrate on the next release.
Use Tailwind's arbitrary-value syntax with a var() fallback so the sidebar renders at a sane width even when the consumer hasn't defined --sidebar-width. Consumers can still override by setting the variable in their own stylesheet.
Replace the non-interactive <span> with a <button>, add aria-expanded and aria-controls pointing at the content <ul>, and keep the ARIA state in sync in the hook on both initial mount and click. Users can now focus the toggle with Tab and expand/collapse it with Enter or Space, fixing WCAG 2.1.1 and 4.1.2 violations.
The mobile sidebar is a modal overlay but was leaking Tab focus into the hidden content underneath and never returned focus to the toggle on close. Store the previously focused element when opening, move focus into the sidebar, trap Tab within it while open, and restore focus on close. Apply role="dialog"/aria-modal dynamically so the desktop collapsible panel is not mislabeled as a modal.
When the sidebar is translated off-canvas, its links and buttons stay in the DOM and keyboard focus can land on invisible elements. Toggle the inert attribute alongside the transform so the off-canvas subtree is removed from the tab order and the accessibility tree.
Server-side renders the sidebar hidden on mobile and visible on desktop via -translate-x-full md:translate-x-0 so the CSS output matches the default state before JS loads. The hook now writes inline transform and margin styles (which win over the Tailwind responsive classes) and clears a data-suppress-transition attribute on the first animation frame so the snap to a stored non-default preference is instant rather than animated on page load. A remaining animation can occur on desktop hard-reload when the user previously collapsed the sidebar, because the server does not yet know the stored preference. A future cookie-backed UI state system will close that gap.
Store bound handlers as instance references during mount so the hook can remove its document keydown, matchMedia change, toggle click, overlay click, and per-section click listeners when the element is destroyed. Matches the cleanup pattern used by every other hook in this directory and prevents handlers from leaking across LiveView navigations.
Attach the BackpexSidebar hook conditionally on the presence of the sidebar slot, and early-return from mounted()/updated() when the sidebar DOM is missing. This prevents a TypeError on layouts that use app_shell without a sidebar.
Change the outer sidebar element from <aside aria-label="Main navigation"> to <nav> and drop the nested <nav> inside the slot. This avoids a double landmark and keeps screen reader navigation unambiguous.
The module was two one-line Plug.Conn wrappers around the already-public Backpex.Preferences.session_key/0, and nothing in the tree called them. The test suite reaches past it and calls get_session(conn, session_key()) directly in nine places; that stays the one way to do it. The preferences guide already documented the renew_session hazard against the raw session key ~600 lines earlier, so shipping the module would give v0.20 two divergent answers to one question. Fold the two details only the module's section carried -- that the generated helper re-puts an allowlist, and that users otherwise lose theme, sidebar state and persisted filters/order/columns on every sign in and out -- into that section and drop the duplicate. The hazard itself is real, and the surviving section remains the single answer: clear_session/1 wipes the whole session, so a phx.gen.auth app silently resets theme and sidebar on login unless the key is carried across. The preference system is unreleased, so removing the module costs no migration.
Neither has a consumer. cookieName() has zero call sites and the hook-authoring recipe points authors at isPending/1 instead; connect_param/0 was only ever read back by its own tautology test, which is redundant with the coverage that already pins "backpex_prefs" behaviorally. Collapse both to the module-private COOKIE_NAME const and @connect_param attribute that already back them. @connect_param and @client_cookie stay separate: they share a value by coincidence, not by contract.
The default-filter redirect is real, but Phoenix.LiveViewTest already
solves it. follow_redirect/2 is a strict superset of what
live_resource_index/3 did: it also handles {:error, {:redirect, _}},
recycles the conn, carries the LiveView connect params across the hop and
re-signs the flash cookie, none of which the bare live(conn, to) re-mount
did. put_preference/3 was a one-line unwrap of the public
Backpex.Preferences.put/4.
Keep the guide's explanation of the redirect and point it at the Phoenix
helper instead, so integrators hitting the footgun still find the answer
without Backpex owning a public test module forever.
Backpex broadcast every preference write on an opt-in PubSub but shipped no subscriber: no handle_info, no on_mount, nothing in InitAssigns reacting to :backpex_preference_changed. A user following the guide verbatim still got a stale second tab, because they also had to write their own on_mount, attach_hook and handle_info and re-assign Backpex's internal assigns. Drop the emitter half rather than freeze a half-loop into a published message contract. Ship the whole loop once there is a consumer. Removes subscribe/1, unsubscribe/1, topic/2 and the private broadcast cluster from Backpex.Preferences, its call sites in put/4 and put_batch/3, the :pubsub config key, and the guide section documenting them. The resource CRUD PubSub is a separate, complete loop and is untouched.
The `validate_keys` gate wrapped every read/write in the preferences
dispatcher, but it did not catch the errors it existed to catch and it
opened one it did not close.
`PreferencesController.update/2` feeds browser-supplied keys straight into
`put_batch/3`, which called `maybe_validate_key/2` with no rescue anywhere
in the path. Under `validate_keys: true` — the setting the guide recommended
for `config/test.exs` — an unauthenticated POST of `{"key": "evil.pwn"}`
raised an ArgumentError out of a public route.
The check also only looked at a key's first segment, so the realistic typo
(`custom.dashbord.view_mode`) validated clean. Every key Backpex owns comes
from a `Backpex.Preferences.Keys` helper, where a typo is already a compile
error, and `custom.*` is an open namespace under the same check — so
`:extra_prefixes` only ever bought a different first word.
Removes the gate and its `:log`/`:raise` strategies, the `:extra_prefixes`
option, the public `Keys.known_prefixes/0`, and the `@after_compile`
self-check on `Keys` (whose per-helper prefix assertion is strictly
dominated by the exact-string assertions in `keys_test.exs`).
`Backpex.Preferences.Key.validate/1` and the built-in prefix list stay:
`Context.put_client/2` uses them, with `Keys.valid_value?/2`, to filter
browser-written keys and values from the connect params and the
`backpex_prefs` cookie before they reach a render. That is a trust boundary,
not a dev-time gate — `inert={not @sidebar_open}` raises on a string, so one
planted cookie value would otherwise 500 every admin page until it expired.
Adds a test pinning the value half of that filter, which until now was only
asserted in a doctest that no test file ran.
`Backpex.Preferences.fetch/3` had no callers: nothing in the library, the
demo or the test suite reached for it, and its own docs retracted the
premise the moduledoc sold it on — `:error` cannot tell "no user
identified" apart from "user has not set this preference yet". `parse_key/1`
was a delegate to the already-public `Backpex.Preferences.Key.parse/1`.
`fetch/3` was, however, the only read that honored the adapter behaviour's
rule that `{:error, :unidentified}` "should be treated as not found". Teach
`get/3` and `get_map/3` that rule so the library stops logging a warning on
a documented-normal path (anonymous visitors, background jobs) — previously
every such read produced operator noise.
Correct the user-preferences guide, which claimed `persist: [:filters]`
followed the `fetch/3` pattern. It never did: it reads with a bare `get/3`
and discriminates on `not is_nil(persisted.filters)`, so an explicitly
cleared `%{}` still beats the filter defaults.
The list in `{:ok, [side_effect()]}` existed because `put_batch/3` has N
entries, not because one `put/4` emits N effects. The Session adapter, the
only real emitter, always returned a one-element list, and every other
adapter returned `[:noop]` — a list holding an atom that means "empty list".
Collapse the per-put return to `{:ok, :persisted} | {:ok, {:put_session,
key, map}}` and drop `:noop`. The key stays inside the effect tuple so the
dispatcher is not coupled to `Adapters.Session`. The batch-level list in
`put_batch/3` and `apply_effects_on_conn/2` is genuine and stays; adapters
that persist on their own now contribute nothing to it instead of padding it
with `:noop`.
`apply_effects_on_socket/5` is inlined into `put/4`, preserving its
trust-boundary guard: a third-party adapter that wrongly returns a
`:put_session` effect from a socket write still gets a warning plus a working
push_event write rather than a CaseClauseError inside a live render.
The router carried three pattern tiers. Two had no consumer outside their own
tests, and both were broken where they would have mattered.
Per-resource routing was inexpressible and failed silently. Patterns were split
on dots while Key.parse/1 splits keys on colons, so a route like
{"resource:MyApp.PostLive:*", Adapter} matched nothing and its keys drifted to
the next-broadest route without a word. Wildcards are now segmented by
Key.parse/1 — the same function that segments keys — so a pattern addresses
exactly the segments a key is built from, and per-resource carve-outs work.
A "*" anywhere but the final segment can never match a key and now raises at
boot instead of matching nothing at runtime.
Match-function patterns are gone. They were split-brain: under the guide's own
example, resolve/2 sent a write to one adapter while resolve_prefix/2 sent the
subtree read to another, and the guide told you to paper over it with a
redundant second route. An adapter now owns a prefix of the key space, which is
what lets a subtree read name a single owner.
validate_no_conflicts!/1 is gone. Resolution ranks by specificity, so the narrow
route already won regardless of declaration order; the check could only reject
configs that would have worked — including the natural per-resource one.
resolve_prefix/2 and its five-tier lattice are gone. They served one call site
reading one literal prefix, where resolve/2 returns the same adapter. get_map/3
now resolves the prefix through the same routes as the keys beneath it, so a
read cannot miss its own writes.
Route shape validation, both route forms, and the zero-config Session default
are unchanged.
Backpex.Preferences.put/4 already asks the key's adapter how a write should
land and only falls back to a browser round-trip when the adapter cannot write
outside an HTTP request cycle. The four index writes (columns, metrics, filters,
order) called Backpex.Preferences.LiveView.push_write/4 instead, which hardcodes
that round-trip — so an app with a server-side adapter paid for a push_event, a
POST and a controller dispatch it never needed.
Route them through put/4. For the Session adapter nothing changes: it still
returns {:error, :requires_http}, the existing fallback still fires, and the
emitted push_event is identical, mirror flag included — put/4 now threads
:mirror into the fallback instead of dropping it. A server-side adapter takes
the write in place and the browser is not involved at all.
Keep :mirror. It says whether a value must survive a live_redirect, which is a
property of the key rather than of the adapter: columns and metrics need it,
filters and order deliberately opt out because they round-trip through the URL.
sidebar_section/1 and sidebar_item/1 each emit a bare <li>, so they need a <ul> parent. That parent lived in sidebar_menu/1, which callers had to remember to wrap around them. Nothing enforced it, and the invariant was already violated in-tree: the user preferences guide puts a sidebar_section straight into <:sidebar>, emitting an orphan <li> that axe-core's listitem rule flags. Commit 7c11103 had to repair the same slip in the installation guide. Make app_shell/1 render the scrollable container and the <ul> around the <:sidebar> slot again, as it did before b99de96, and delete sidebar_menu/1. Branding — the reason the <ul> was flattened out, since it has to sit above the scroll container — moves into its own <:sidebar_branding> slot, rendered inside the same <nav> so it stays within the focus trap and the inert region. has_sidebar now covers a branding-only sidebar. The rendered DOM is unchanged, so inert, the mobile focus trap, aria-expanded/aria-controls and the single <nav> landmark all behave as before. The guide snippet that was broken needs no edit: a sidebar_section directly inside <:sidebar> is now exactly right.
The Context doctests were never declared in a test file, so the examples documenting put_client/2 had never executed. Declare them, and add the example that keeps the two rejection cases honest. put_client/2 filters browser-written preference keys and values before a read overlays them on a render. The existing examples cover a key with an unknown prefix and a wrong-typed value for a built-in key, but both assert that an entry is absent from the result. A shape gate that rejected every value for a key would satisfy both while silently dropping the user's preference on every read, so also assert that a well-shaped value survives.
The order criterion built from init_order carried no :field_name key, but
the Ecto adapter's order clause required one. The clause never matched, so
the criterion fell through to the catch-all and the ORDER BY was dropped.
Because init_order defaults to %{by: :id, direction: :asc} and a primary key
is virtually never a declared field, this affected nearly every LiveResource:
index queries ran LIMIT/OFFSET with no ORDER BY, which Postgres is free to
answer in any order, so paging could repeat or skip rows.
Build the criterion with a :field_name and match any {:order, _} in the
adapter, raising on a malformed order map rather than quietly returning
unordered rows.
Short links key on :short_key and have no id column, so they now declare
their own init_order. Demo ordering tests that asserted insertion order
under the id default were only reading heap order; they now assert the real
id-ascending order of the records they insert.
The Session adapter warned at 3072 bytes but measured only Backpex's own
subtree in raw term form. The cookie that actually ships is the whole
session, signed and Base64'd, sharing a 4096 byte budget with whatever the
host app keeps there. The warning could not fire before the break, and the
break was a CookieOverflowError 500 on every subsequent request.
Measure the whole prospective session at its encoded size and give the put
callback a way to refuse: `{:error, :too_large}` leaves the stored value
untouched and surfaces as 422, which the browser can act on. Warn at 75% of
the budget, where there is still room to react. `:max_bytes` defaults to the
cookie store's 4096 and takes `:infinity` for server-side session stores,
which have no such cap.
Client-side, a 5xx no longer retires a pending write: the server crashed
rather than ruling on it, so it stays pending and is replayed. Previously
any completed response cleared it, so an overflow 500 looked like success
and the value silently reverted on the next full load.
The write path applied no shape validation to built-in keys, so a wrong-typed
value persisted and then raised on every later render — `not "false"` for
sidebar_open, Map.get/3 on a binary for columns. That 500s every admin page
for the life of the user's session, and because every page 500s there is no
way to reach one to undo it.
The ephemeral overlay already gated on exactly this (Context.put_client/2
calls Keys.valid_value?/2, because a render must not raise on browser input).
The durable path was the weaker of the two despite being the one that
persists. Move the gate into dispatch_put/4, the choke point every write
funnels through, so the endpoint, LiveView writes and host-app writes all get
it. Refusals surface as 422 {reason: "invalid_value"}.
This is a shape gate, not authorization: keys Backpex does not own (custom.*,
unknown resource: suffixes) have no known shape and still pass through
untouched, so host apps lose no flexibility.
Guard the two readers as well — a store may already hold a poisoned value
from an earlier version or a third-party adapter, and neither reader should
be able to take a page down.
maybe_persist_order/2 runs from the render pipeline, not an event handler. On a first visit nothing is stored, so the derived order differed from `nil` and the resource's own init_order got written back as though the user had chosen it. From the next mount on the stored value wins over init_order. That froze the default per user at whatever it resolved to on their first page view: change the resource's default later and existing users never see it, and a fun/1 init_order silently stopped being dynamic after one render. There was no way back to "unset" — only picking some other explicit order overwrote it. It also meant passive browsing wrote preferences: one POST per persisted resource just for viewing the index. Write only when the effective order diverges from the resource's own default, which is what makes it a choice worth recording. While it still matches, `nil` reads back as that same default, so storing it changes no outcome — it only freezes it. The URL cannot be the signal: the default-filter redirect canonicalizes order_by/order_direction into it, so a plain first visit arrives with an order in the params that nobody chose. Compare against the resolved init_order from before the stored-order override instead. The repair path for a stored order naming a dropped column is unchanged.
In v0.19 theme_selector/1 was self-contained: its form carried data-cookie-path, so it worked in any layout. In v0.20 the endpoint is rendered in exactly one place — the #backpex-preferences div inside app_shell/1 — and that is the only element the JS hook reads it from. app_shell therefore became an undeclared prerequisite for every preference write. An app bringing its own layout — which Backpex explicitly supports — lost all preference persistence on upgrade with no compile error (theme_selector no longer declares socket, so the old call site still compiles) and no test failure. The UI updates optimistically, so it looked fine until the next reload reverted it. Extract the element into a public Backpex.HTML.Layout.preferences_root/1 that app_shell renders internally, and document it as required for any other layout — in the v0.20 migration guide next to the theme_selector step, and in the user-preferences guide. Name the missing element in the console warning rather than an internal variable. The existing layout test rendered theme_selector standalone and asserted markup only, so a component that renders correctly and does nothing at runtime passed. Assert app_shell actually wires the endpoint.
Toggling the sidebar and reloading inside the persist POST's round-trip window paints the pre-toggle state, and the hook then corrects it from the sessionStorage mirror once it mounts. That correction is meant to be invisible: `data-suppress-transition` is server-rendered so it snaps rather than slides. It slid anyway. requestAnimationFrame runs *before* the frame's style recalculation, so the guard was lifted in the very style-change event that applied the correction. A transition starts from the after-change style, so a suppressed before-change style buys nothing: the browser saw the translate change and a live 300ms transition-property in the same event and animated between them. The sidebar visibly slid open->closed, and #backpex-main animated its margin alongside it. That 300ms slide is the reported flash. Force the corrected state through a style recalculation while it is still suppressed, so it becomes the before-change style, then release the guard against an unchanged value. Doing this synchronously also fixes a second defect: rAF never fires in a background tab, so a sidebar restored there kept the guard forever and could not animate for the rest of the page load.
`data-suppress-transition` is static markup, so morphdom morphs it back onto #backpex-sidebar and #backpex-main on any patch that re-renders the shell — a live_patch from sorting, filtering or paginating is enough. Only mounted() ever took it off, so the first sort silently killed sidebar and main transitions for the rest of the page load. live_redirect hides this: it replaces the nodes and re-mounts the hook, which releases the guard again. The in-place patch is the path that strands it. Release it from updated() as well. The hasAttribute check keeps this free on the patches that don't restore it.
Pins the regression fixed in the previous commit: sorting re-renders the shell in place, morphdom morphs `data-suppress-transition` back on, and before the fix nothing took it off again. Asserts against the in-place patch specifically. The existing live_redirect tests cannot catch this — they replace the nodes and re-mount the hook, which releases the guard as a side effect.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
collapsible_sidebar.mov