diff --git a/crates/rest/src/handlers/user_settings.rs b/crates/rest/src/handlers/user_settings.rs index 4eb8392b7..76c74292a 100644 --- a/crates/rest/src/handlers/user_settings.rs +++ b/crates/rest/src/handlers/user_settings.rs @@ -326,22 +326,28 @@ fn parse_if_match(conditional: &ConditionalHeaders) -> RestResult RestResult<()> { - let current = (version > 0).then(|| version.to_string()); - if precondition.if_match_satisfied(current.as_deref()) { + let satisfied = match precondition { + EntityTagPrecondition::Tags(_) => { + precondition.if_match_satisfied(Some(&version.to_string())) + } + _ => precondition.if_match_satisfied((version > 0).then(|| version.to_string()).as_deref()), + }; + if satisfied { return Ok(()); } - let message = match current { - Some(current) => { - format!("If-Match precondition failed: current settings version is {current}") - } - None => "If-Match precondition failed: no settings document exists yet".to_string(), + let message = if version > 0 { + format!("If-Match precondition failed: current settings version is {version}") + } else { + "If-Match precondition failed: no settings document exists yet".to_string() }; Err(RestError::PreconditionFailed { message }) } diff --git a/crates/rest/tests/user_settings.rs b/crates/rest/tests/user_settings.rs index 68d1f90f2..93f7b3604 100644 --- a/crates/rest/tests/user_settings.rs +++ b/crates/rest/tests/user_settings.rs @@ -119,6 +119,45 @@ async fn patch_null_deletes_a_key() { assert_eq!(get.json::(), json!({"theme": "dark"})); } +/// #442: `GET` serves the empty document as a real representation (`{}` with +/// `ETag: "0"`), so the validator it hands out must round-trip — a fresh +/// user's read-then-conditional-write deadlocked on 412 forever otherwise. +#[tokio::test] +async fn if_match_zero_writes_the_first_version() { + let server = create_test_server(); + + let fresh = server.get("/_user/settings").await; + assert_eq!(fresh.header("etag"), HeaderValue::from_static("\"0\"")); + + let created = server + .patch("/_user/settings") + .add_header(IF_MATCH, HeaderValue::from_static("\"0\"")) + .json(&json!({"recentSearches": [{"query": "/Patient?name=x"}]})) + .await; + created.assert_status_ok(); + assert_eq!(created.header("etag"), HeaderValue::from_static("\"1\"")); + + // And once a version exists, "0" is genuinely stale again. + let stale = server + .patch("/_user/settings") + .add_header(IF_MATCH, HeaderValue::from_static("\"0\"")) + .json(&json!({"a": 1})) + .await; + assert_eq!(stale.status_code(), StatusCode::PRECONDITION_FAILED); +} + +/// `If-Match: *` still requires an actually stored document. +#[tokio::test] +async fn if_match_star_still_requires_an_existing_document() { + let server = create_test_server(); + let conflict = server + .patch("/_user/settings") + .add_header(IF_MATCH, HeaderValue::from_static("*")) + .json(&json!({"a": 1})) + .await; + assert_eq!(conflict.status_code(), StatusCode::PRECONDITION_FAILED); +} + #[tokio::test] async fn stale_if_match_is_rejected_with_412() { let server = create_test_server(); diff --git a/crates/ui/assets/app.css b/crates/ui/assets/app.css index 834e88ec9..9e1e7d8e3 100644 --- a/crates/ui/assets/app.css +++ b/crates/ui/assets/app.css @@ -101,11 +101,25 @@ body { margin: 0; min-height: 100vh; display: grid; - grid-template-columns: 295px 1fr; + grid-template-columns: 76px 1fr; font-size: 15px; line-height: 1.2; } +/* The fixed sidebar sits outside the grid flow, so the flow columns need + explicit placement — otherwise the pane auto-fills the rail column. */ +.pane { + grid-column: 2; +} + +body.has-nav-panel .nav-panel { + grid-column: 2; +} + +body.has-nav-panel .pane { + grid-column: 3; +} + svg { display: block; } @@ -119,13 +133,29 @@ svg { /* ---------- Sidebar ---------- */ .sidebar { + position: fixed; + top: 0; + bottom: 0; + left: 0; + z-index: 40; + box-sizing: border-box; + width: 295px; display: flex; flex-direction: column; gap: 20px; padding: 20px 24px; border-right: 1px solid var(--divider); box-shadow: 1px 0 0 var(--divider-glint); - min-height: 100vh; + background: var(--bg); + overflow-y: auto; + overflow-x: hidden; + transition: width 0.15s ease; +} + +/* Expanded (hover / keyboard focus): overlay the content with a lift shadow. */ +.sidebar:hover, +.sidebar:focus-within { + box-shadow: 0 8px 32px rgba(0, 0, 0, 0.14); } .brand { @@ -218,71 +248,36 @@ a.nav-item:hover { white-space: nowrap; } -/* ---------- Collapse toggle + collapsed (icon-only) rail ---------- */ +/* ---------- Collapsed (icon-only) rail ---------- */ .brand__text { flex: 1; min-width: 0; } -.nav-toggle { - display: grid; - place-items: center; - width: 32px; - height: 32px; - flex: none; - padding: 0; - border: 0; - border-radius: 8px; - background: transparent; - color: var(--muted); - cursor: pointer; -} - -.nav-toggle:hover { - background: var(--accent-soft); - color: var(--text-strong); -} - /* - * The toggle icon is directional (a panel + chevron). The base glyph points - * right (= expand), which is correct for the collapsed rail; while expanded, - * mirror it so the chevron points left (= collapse). + * Collapsed rail: the resting state, whenever the pointer and the keyboard + * focus are both elsewhere (#438 dropped the explicit toggle). Labels are + * visually hidden but kept in the accessibility tree so screen readers still + * announce them; a `title` on each item gives a hover tooltip, and hovering + * or focusing the rail expands it in place. */ -.nav-toggle .icon { - transform: scaleX(-1); -} - -html[data-nav="collapsed"] .nav-toggle .icon { - transform: none; -} - -/* - * Collapsed rail: driven by `data-nav="collapsed"` on (set by nav.js - * before first paint), and forced on narrow viewports (see the media query - * below). Labels are visually hidden but kept in the accessibility tree so - * screen readers still announce them; a `title` on each item gives a hover - * tooltip. - */ -html[data-nav="collapsed"] body { - grid-template-columns: 76px 1fr; -} - -html[data-nav="collapsed"] .sidebar { +.sidebar:not(:hover):not(:focus-within) { + width: 76px; padding: 20px 14px; align-items: center; } -html[data-nav="collapsed"] .brand { +.sidebar:not(:hover):not(:focus-within) .brand { flex-direction: column; gap: 10px; } -html[data-nav="collapsed"] .brand__text { +.sidebar:not(:hover):not(:focus-within) .brand__text { display: none; } -html[data-nav="collapsed"] .nav__section { +.sidebar:not(:hover):not(:focus-within) .nav__section { /* Group headers are decorative; drop them entirely when collapsed. */ height: 1px; margin: 8px 0; @@ -291,23 +286,23 @@ html[data-nav="collapsed"] .nav__section { border-top: 1px solid var(--divider); } -html[data-nav="collapsed"] .nav-item { +.sidebar:not(:hover):not(:focus-within) .nav-item { width: 48px; padding: 0; justify-content: center; } -html[data-nav="collapsed"] .selector { +.sidebar:not(:hover):not(:focus-within) .selector { width: 48px; padding: 8px; justify-content: center; } /* Hide the text of labels/selectors without removing them from the a11y tree. */ -html[data-nav="collapsed"] .nav-item__label, -html[data-nav="collapsed"] .selector__label, -html[data-nav="collapsed"] .selector__chevrons, -html[data-nav="collapsed"] .brand__version { +.sidebar:not(:hover):not(:focus-within) .nav-item__label, +.sidebar:not(:hover):not(:focus-within) .selector__label, +.sidebar:not(:hover):not(:focus-within) .selector__chevrons, +.sidebar:not(:hover):not(:focus-within) .brand__version { position: absolute; width: 1px; height: 1px; @@ -1048,65 +1043,6 @@ button.pill { /* ---------- Narrow screens: stack the shell ---------- */ @media (max-width: 900px) { - /* - Narrow viewports force the collapsed icon rail regardless of the persisted - preference. Reuse the collapsed-state rules by mapping them onto the base - selectors here (the `data-nav` attribute still controls wide screens). - */ - body { - grid-template-columns: 76px 1fr; - } - - .sidebar { - padding: 20px 14px; - align-items: center; - } - - .brand { - flex-direction: column; - gap: 10px; - } - - .brand__text, - .nav-toggle { - display: none; - } - - .nav__section { - height: 1px; - margin: 8px 0; - overflow: hidden; - text-indent: -999px; - border-top: 1px solid var(--divider); - } - - .nav-item { - width: 48px; - padding: 0; - justify-content: center; - } - - .selector { - width: 48px; - padding: 8px; - justify-content: center; - } - - .nav-item__label, - .selector__label, - .selector__chevrons, - .brand__version { - position: absolute; - width: 1px; - height: 1px; - padding: 0; - margin: -1px; - overflow: hidden; - clip: rect(0, 0, 0, 0); - white-space: nowrap; - border: 0; - } - .stat-grid { grid-template-columns: repeat(2, 1fr); } @@ -3574,7 +3510,7 @@ button.filter-rail__item { /* Three columns when a nav panel is present: sidebar, the panel, content. */ body.has-nav-panel { - grid-template-columns: 250px 232px 1fr; + grid-template-columns: 76px 232px 1fr; } .nav-panel { @@ -3667,7 +3603,8 @@ body.has-nav-panel { } @media (max-width: 1100px) { - body.has-nav-panel { grid-template-columns: 250px 1fr; } + body.has-nav-panel { grid-template-columns: 76px 1fr; } + body.has-nav-panel .pane { grid-column: 2; } .nav-panel { display: none; } } @@ -3688,18 +3625,6 @@ body.has-nav-panel { font-weight: 500; } -/* --------------------------------------------------------------------------- - Collapsed sidebar + the Resources type panel. The sidebar collapse itself is - driven by `data-nav="collapsed"` (the block near the top of this file). Here - we only add the 3-column grid variant: when the type panel is present, the - collapsed sidebar rail must keep the panel and content in their own columns - (otherwise the content wraps below the two rails). - --------------------------------------------------------------------------- */ -html[data-nav="collapsed"] body.has-nav-panel { - grid-template-columns: 76px 232px 1fr; -} -html[data-nav="collapsed"] .menu__panel { left: 0; } - /* OR value stacks (#414): one input per comma alternative. */ .builder-row__values { display: flex; diff --git a/crates/ui/assets/nav.js b/crates/ui/assets/nav.js deleted file mode 100644 index d2dabb483..000000000 --- a/crates/ui/assets/nav.js +++ /dev/null @@ -1,97 +0,0 @@ -// Collapsible primary navigation for the HFS web UI (design: Figma "Search -// V1.0" collapsed rail). -// -// Loaded from WITHOUT `defer`, deliberately: the collapse state must be -// set on before first paint or an expanded sidebar flashes to collapsed. -// -// The choice roams across devices via the per-user settings document -// (`/_user/settings`, the same opaque JSON store the theme uses); localStorage -// stays as a fast-path cache so first paint never waits on the network. -// Precedence: server setting -> localStorage cache -> expanded default. When the -// settings endpoint is unavailable, behavior degrades to localStorage-only. -// -// The toggle button carries `data-toggle-nav`; a single delegated listener keeps -// behavior in one pinned asset (see README.md: no inline script blobs). Narrow -// viewports are forced collapsed by CSS regardless of this preference. -(function () { - var KEY = "hfs-nav"; - var SETTINGS = "/_user/settings"; - - function valid(state) { - return state === "collapsed" || state === "expanded" ? state : null; - } - - function apply(state) { - document.documentElement.setAttribute("data-nav", state); - // Keep the toggle's ARIA state in sync (the button reflects the *expanded* - // status of the navigation it controls). - var buttons = document.querySelectorAll("[data-toggle-nav]"); - for (var i = 0; i < buttons.length; i++) { - buttons[i].setAttribute("aria-expanded", state === "expanded" ? "true" : "false"); - } - } - - function cache(state) { - try { - localStorage.setItem(KEY, state); - } catch (e) { - /* non-fatal: the choice just won't persist on this device */ - } - } - - // 1. First paint: cache -> expanded default (synchronous, before render). - var cached = null; - try { - cached = valid(localStorage.getItem(KEY)); - } catch (e) { - /* storage may be unavailable (e.g. blocked); fall through */ - } - document.documentElement.setAttribute("data-nav", cached || "expanded"); - - // Sync the toggle's aria-expanded once the DOM is ready (the button doesn't - // exist yet at this point in ). - document.addEventListener("DOMContentLoaded", function () { - apply(document.documentElement.getAttribute("data-nav") || "expanded"); - }); - - // 2. Reconcile with the server-side settings document: a roamed choice from - // another device wins over the local cache. - if (window.fetch) { - fetch(SETTINGS, { headers: { Accept: "application/json" }, credentials: "same-origin" }) - .then(function (response) { - return response.ok ? response.json() : null; - }) - .then(function (doc) { - var server = doc && valid(doc.nav); - if (!server) return; - if (server !== document.documentElement.getAttribute("data-nav")) apply(server); - cache(server); - }) - .catch(function () { - /* settings unavailable: keep behaving like the cache-only model */ - }); - } - - // 3. Toggle: flip, apply immediately, cache locally, persist to the settings - // document with an RFC 7386 merge-patch so only the nav key changes. - document.addEventListener("click", function (event) { - var button = event.target.closest && event.target.closest("[data-toggle-nav]"); - if (!button) return; - var current = document.documentElement.getAttribute("data-nav") === "collapsed" - ? "collapsed" - : "expanded"; - var next = current === "collapsed" ? "expanded" : "collapsed"; - apply(next); - cache(next); - if (window.fetch) { - fetch(SETTINGS, { - method: "PATCH", - headers: { "Content-Type": "application/json" }, - credentials: "same-origin", - body: JSON.stringify({ nav: next }), - }).catch(function () { - /* non-fatal: the local cache still holds the choice */ - }); - } - }); -})(); diff --git a/crates/ui/e2e/pages/chrome.ts b/crates/ui/e2e/pages/chrome.ts index 82c3c506e..dd907525a 100644 --- a/crates/ui/e2e/pages/chrome.ts +++ b/crates/ui/e2e/pages/chrome.ts @@ -1,17 +1,15 @@ -// The persistent app chrome (layouts/base.html): sidebar nav, collapse toggle, -// theme buttons, and the language switcher. Present on every full page. +// The persistent app chrome (layouts/base.html): the sidebar rail (expands on +// hover, #438), theme buttons, and the language switcher. On every full page. import type { Page, Locator } from "@playwright/test"; export type Theme = "light" | "dark"; export class AppChrome { readonly sidebar: Locator; - readonly navToggle: Locator; readonly langSwitcher: Locator; constructor(readonly page: Page) { this.sidebar = page.locator("aside.sidebar"); - this.navToggle = page.locator("[data-toggle-nav]"); this.langSwitcher = page.locator(".lang-switcher"); } @@ -46,8 +44,4 @@ export class AppChrome { langLink(lang: string): Locator { return this.page.locator(`.lang-switcher a[href*='lang=${lang}']`); } - - async collapsed(): Promise { - return (await this.page.locator("html").getAttribute("data-nav")) === "collapsed"; - } } diff --git a/crates/ui/e2e/pages/fixtures.ts b/crates/ui/e2e/pages/fixtures.ts index 26f0e7a0f..5698989ea 100644 --- a/crates/ui/e2e/pages/fixtures.ts +++ b/crates/ui/e2e/pages/fixtures.ts @@ -25,6 +25,19 @@ type Fixtures = { }; export const test = base.extend({ + // The sidebar expands on hover (#438) and the mouse starts at (0,0) — over + // the rail — so a fresh page would open with the sidebar overlaying the + // left content edge and intercepting clicks. Park the pointer in the topbar + // after every navigation; tests that exercise the hover do so explicitly. + page: async ({ page }, use) => { + const goto = page.goto.bind(page); + page.goto = (async (url: string, opts?: Parameters[1]) => { + const response = await goto(url, opts); + await page.mouse.move(700, 8); + return response; + }) as typeof page.goto; + await use(page); + }, chrome: async ({ page }, use) => use(new AppChrome(page)), dashboard: async ({ page }, use) => use(new DashboardPage(page)), resources: async ({ page }, use) => use(new ResourcesPage(page)), diff --git a/crates/ui/e2e/tests/chrome.spec.ts b/crates/ui/e2e/tests/chrome.spec.ts index 97feb498b..3afa667aa 100644 --- a/crates/ui/e2e/tests/chrome.spec.ts +++ b/crates/ui/e2e/tests/chrome.spec.ts @@ -1,49 +1,48 @@ import { test, expect } from "../pages/fixtures"; -// The persistent chrome: the collapsible nav. Like the theme, it caches in -// localStorage and roams via a /_user/settings merge-patch — behavior only the -// browser can observe. - -test("the nav toggle collapses and expands, syncing aria and the cache", async ({ - page, - chrome, -}) => { +// The persistent chrome: the sidebar rail (#438). There is no toggle and no +// persisted state anymore — the sidebar rests as an icon rail and expands as +// an overlay while hovered or keyboard-focused. + +test("the sidebar rests as a rail and expands on hover", async ({ page, chrome }) => { + await page.goto("/ui", { waitUntil: "networkidle" }); + + // Resting: rail width, labels visually hidden (still in the a11y tree). + await page.mouse.move(800, 400); + await expect.poll(async () => (await chrome.sidebar.boundingBox())?.width).toBeLessThan(100); + + // Hover: expands past the rail and the labels become visible. + await chrome.sidebar.hover(); + await expect.poll(async () => (await chrome.sidebar.boundingBox())?.width).toBeGreaterThan(250); + await expect(chrome.navLink("/ui/resources").locator(".nav-item__label")).toBeVisible(); + + // Content does not reflow: the main column starts at the rail edge and + // stays there while the sidebar overlays it. + const main = await page.locator(".pane").boundingBox(); + expect(main && main.x).toBeGreaterThan(60); + expect(main && main.x).toBeLessThan(120); + + // Leave: collapses back. + await page.mouse.move(800, 400); + await expect.poll(async () => (await chrome.sidebar.boundingBox())?.width).toBeLessThan(100); +}); + +test("keyboard focus inside the sidebar also expands it", async ({ page, chrome }) => { + await page.goto("/ui", { waitUntil: "networkidle" }); + await page.mouse.move(800, 400); + await chrome.navLink("/ui/resources").focus(); + await expect.poll(async () => (await chrome.sidebar.boundingBox())?.width).toBeGreaterThan(250); +}); + +test("there is no expand/collapse toggle", async ({ page }) => { await page.goto("/ui", { waitUntil: "networkidle" }); - // Default is expanded. - await expect(page.locator("html")).toHaveAttribute("data-nav", "expanded"); - await expect(chrome.navToggle).toHaveAttribute("aria-expanded", "true"); - - const patch = page.waitForRequest( - (r) => r.url().endsWith("/_user/settings") && r.method() === "PATCH", - ); - await chrome.navToggle.click(); - - await expect(page.locator("html")).toHaveAttribute("data-nav", "collapsed"); - await expect(chrome.navToggle).toHaveAttribute("aria-expanded", "false"); - const req = await patch; - expect(JSON.parse(req.postData() ?? "{}")).toEqual({ nav: "collapsed" }); - expect(await page.evaluate(() => localStorage.getItem("hfs-nav"))).toBe("collapsed"); - - // Toggling back expands again. - await chrome.navToggle.click(); - await expect(page.locator("html")).toHaveAttribute("data-nav", "expanded"); + await expect(page.locator("[data-toggle-nav]")).toHaveCount(0); }); -test("a returning user's collapsed choice is applied before paint", async ({ page }) => { - // Isolate the cache path: the settings doc is shared across tests (no auth → - // one anonymous user), so pin the server response to empty and let the - // localStorage cache be the sole source, the way a first-visit-elsewhere - // returning user has it. - await page.route("**/_user/settings", (route) => - route.request().method() === "GET" - ? route.fulfill({ contentType: "application/json", body: "{}" }) - : route.fulfill({ status: 200, body: "{}" }), - ); - await page.addInitScript(() => { - try { - localStorage.setItem("hfs-nav", "collapsed"); - } catch {} - }); - await page.goto("/ui", { waitUntil: "domcontentloaded" }); - await expect(page.locator("html")).toHaveAttribute("data-nav", "collapsed"); +test("the Batch & Data section lists Import and Export", async ({ page, chrome }) => { + await page.goto("/ui", { waitUntil: "networkidle" }); + await chrome.sidebar.hover(); + await expect(chrome.soonItem("Import")).toBeVisible(); + await expect(chrome.soonItem("Export")).toBeVisible(); + await expect(chrome.soonItem("SQL-on-FHIR")).toBeVisible(); }); diff --git a/crates/ui/e2e/tests/nojs/progressive-enhancement.spec.ts b/crates/ui/e2e/tests/nojs/progressive-enhancement.spec.ts index 9b5ea17aa..1bf1cd28f 100644 --- a/crates/ui/e2e/tests/nojs/progressive-enhancement.spec.ts +++ b/crates/ui/e2e/tests/nojs/progressive-enhancement.spec.ts @@ -32,6 +32,12 @@ const NAV = [ for (const { href, url } of NAV) { test(`nav link ${href} navigates`, async ({ page, chrome }) => { await page.goto("/ui"); + // Enter the rail and let it finish expanding (#438) so the link's position + // is stable before the click — pure CSS, so it works with JS disabled too. + await chrome.sidebar.hover(); + await expect + .poll(async () => (await chrome.sidebar.boundingBox())?.width) + .toBeGreaterThan(290); const link = chrome.navLink(href); await expect(link).toBeVisible(); await link.click(); diff --git a/crates/ui/src/lib.rs b/crates/ui/src/lib.rs index c79c672b0..4d903ba94 100644 --- a/crates/ui/src/lib.rs +++ b/crates/ui/src/lib.rs @@ -1614,25 +1614,11 @@ mod tests { #[test] fn design_assets_are_embedded() { assert!(Assets::get("theme.js").is_some()); - assert!(Assets::get("nav.js").is_some()); assert!(Assets::get("fonts/figtree-latin.woff2").is_some()); assert!(Assets::get("fonts/figtree-latin-ext.woff2").is_some()); assert!(Assets::get("logo.png").is_some()); } - /// The collapsible-nav script persists the state to the per-user settings - /// document (like the theme, #197): read on load, merge-patch `nav` on - /// toggle, with a localStorage first-paint cache. Guards the wiring. - #[test] - fn nav_script_is_wired_to_user_settings() { - let file = Assets::get("nav.js").expect("nav.js embedded"); - let source = std::str::from_utf8(&file.data).expect("nav.js is UTF-8"); - assert!(source.contains("/_user/settings")); - assert!(source.contains("PATCH")); - assert!(source.contains("hfs-nav"), "localStorage cache stays"); - assert!(source.contains("data-nav"), "sets the collapse attribute"); - } - /// The theme script persists the choice to the per-user settings document /// (#197): it must read the document on load and merge-patch `theme` on /// toggle, with localStorage kept as the first-paint cache. Guards the diff --git a/crates/ui/templates/icons/import.svg b/crates/ui/templates/icons/import.svg new file mode 100644 index 000000000..0ab8e1ae8 --- /dev/null +++ b/crates/ui/templates/icons/import.svg @@ -0,0 +1,3 @@ + + + diff --git a/crates/ui/templates/layouts/base.html b/crates/ui/templates/layouts/base.html index 85b88b25f..dcce67a70 100644 --- a/crates/ui/templates/layouts/base.html +++ b/crates/ui/templates/layouts/base.html @@ -7,10 +7,9 @@ - + - @@ -22,22 +21,6 @@ {{ i18n.t("app-title") }} hfs v{{ status.version }} - -