diff --git a/README.md b/README.md index 6bf231e..cf8f5ea 100644 --- a/README.md +++ b/README.md @@ -81,7 +81,7 @@ Everything is namespaced under `/api/v1/`. `/api/*` outside the version prefix i | Method | Path | Description | | ------ | ------------------------------------------------------- | ------------------------------------------ | | GET | `/api/v1/health` | Liveness probe. | -| GET | `/api/v1/mangas?search=&sort=recent|title&limit=&offset=` | List/search mangas. Trigram fuzzy search. | +| GET | `/api/v1/mangas?search=&sort=created\|updated\|title\|author&order=asc\|desc&limit=&offset=` | List/search mangas. Trigram fuzzy search. `sort` defaults to `updated`; `order` defaults per field (dates `desc`, text `asc`). `sort=recent` is a back-compat alias for `created`. | | GET | `/api/v1/mangas/{id}` | Single manga. | | GET | `/api/v1/mangas/{id}/chapters` | Paginated chapter list, ordered by number. | | GET | `/api/v1/mangas/{id}/chapters/{n}` | Single chapter. | diff --git a/backend/migrations/0033_mangas_updated_at_index.sql b/backend/migrations/0033_mangas_updated_at_index.sql new file mode 100644 index 0000000..f60da8b --- /dev/null +++ b/backend/migrations/0033_mangas_updated_at_index.sql @@ -0,0 +1,16 @@ +-- Back the default catalog ordering. Since 0.88.0 the catalog list defaults to +-- `ORDER BY updated_at DESC, id` (the no-filter default path), but the only +-- existing updated_at index on mangas is the PARTIAL `mangas_missing_cover_idx` +-- (WHERE cover_image_path IS NULL), which Postgres can't use for the unfiltered +-- ordering. `created_at` and `lower(title)` already have full indexes from +-- 0001; this gives the new default sort column the same treatment. +-- +-- `id` is included as a trailing key so the index also covers the stable +-- `, id` tie-break the listing appends for deterministic pagination. +-- +-- Not CONCURRENTLY: sqlx::migrate! wraps each migration in a transaction, and +-- CREATE INDEX CONCURRENTLY can't run inside one. The table is small at our +-- scale, so a brief lock on an online deploy is acceptable. + +CREATE INDEX IF NOT EXISTS mangas_updated_at_idx + ON mangas (updated_at DESC, id); diff --git a/backend/src/api/mangas.rs b/backend/src/api/mangas.rs index 93ab31d..2533773 100644 --- a/backend/src/api/mangas.rs +++ b/backend/src/api/mangas.rs @@ -51,16 +51,58 @@ pub struct ListParams { pub limit: i64, #[serde(default)] pub offset: i64, + /// Sort field: `created`, `updated` (default), `title`, or `author`. + /// Also accepts the legacy `recent` alias (== `created`). Anything + /// else → 422. #[serde(default)] - pub sort: repo::manga::SortField, + pub sort: Option, + /// `asc` or `desc`. Omitted → the field's natural default + /// (dates `desc`, text `asc`). Anything else → 422. #[serde(default)] - pub order: repo::manga::SortOrder, + pub order: Option, } fn default_limit() -> i64 { 50 } +/// Parse the `sort` wire value into a [`repo::manga::SortField`]. Validation +/// and the legacy `recent` alias live here (not in serde) so unknown values +/// return the structured 422 envelope rather than axum's plain-text +/// `QueryRejection`, matching the rest of this handler's filters. +fn parse_sort(raw: Option<&str>) -> AppResult { + use repo::manga::SortField; + match raw.map(str::trim) { + None | Some("") | Some("updated") => Ok(SortField::Updated), + // `recent` was the pre-0.88 name for "newest created first"; keep it + // as an alias so existing bots/scripts don't break. + Some("created") | Some("recent") => Ok(SortField::Created), + Some("title") => Ok(SortField::Title), + Some("author") => Ok(SortField::Author), + Some(other) => Err(AppError::ValidationFailed { + message: format!( + "sort must be one of created, updated, title, author (got {other:?})" + ), + details: json!({ "sort": "invalid" }), + }), + } +} + +/// Parse the `order` wire value. `None` means the client omitted it, so the +/// caller applies the field's natural default. An explicit unknown value → 422. +fn parse_order(raw: Option<&str>) -> AppResult> { + use repo::manga::SortOrder; + match raw.map(str::trim) { + None | Some("") => Ok(None), + Some("asc") => Ok(Some(SortOrder::Asc)), + Some("desc") => Ok(Some(SortOrder::Desc)), + Some(other) => Err(AppError::ValidationFailed { + message: format!("order must be 'asc' or 'desc' (got {other:?})"), + details: json!({ "order": "invalid" }), + }), + } +} + fn parse_uuid_csv(field: &str, raw: Option<&str>) -> AppResult> { let Some(raw) = raw else { return Ok(Vec::new()) }; let mut out = Vec::new(); @@ -96,6 +138,10 @@ async fn list( Some(s.to_string()) } }; + let sort = parse_sort(params.sort.as_deref())?; + // Omitted `order` falls back to the field's natural direction, so a bare + // `?sort=title` reads A→Z just like the UI shows it. + let order = parse_order(params.order.as_deref())?.unwrap_or_else(|| sort.default_order()); let q = repo::manga::ListQuery { search: params.search.filter(|s| !s.trim().is_empty()), status, @@ -106,8 +152,8 @@ async fn list( cw_exclude: crate::api::page_tags::parse_warnings_csv(params.cw_exclude.as_deref())?, limit, offset, - sort: params.sort, - order: params.order, + sort, + order, }; let (items, total) = repo::manga::list_cards(&state.db, &q).await?; Ok(Json(PagedResponse::with_total(items, limit, offset, total))) diff --git a/backend/src/repo/manga.rs b/backend/src/repo/manga.rs index 1734384..fd259c9 100644 --- a/backend/src/repo/manga.rs +++ b/backend/src/repo/manga.rs @@ -5,7 +5,6 @@ //! handlers depend only on `sqlx::PgPool`, not on a trait object. Swap to //! a trait + impl if a second backend ever becomes necessary. -use serde::Deserialize; use sqlx::{PgConnection, PgExecutor, PgPool}; use uuid::Uuid; @@ -19,9 +18,10 @@ pub const STATUSES: &[&str] = &["ongoing", "completed"]; pub const DEFAULT_STATUS: &str = "ongoing"; /// Which column the listing is ordered by. The direction lives in a separate -/// [`SortOrder`] so any field can be sorted either way. -#[derive(Debug, Clone, Copy, Default, Deserialize)] -#[serde(rename_all = "snake_case")] +/// [`SortOrder`] so any field can be sorted either way. Wire values are parsed +/// (with validation and the legacy `recent` alias) in `api::mangas`, not via +/// serde, so this stays a plain domain enum. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub enum SortField { /// Creation time. Created, @@ -35,10 +35,21 @@ pub enum SortField { Author, } -/// Sort direction. Defaults to descending so the default listing -/// (`Updated`/`Desc`) shows the most recently updated mangas first. -#[derive(Debug, Clone, Copy, Default, Deserialize)] -#[serde(rename_all = "snake_case")] +impl SortField { + /// The direction applied when the client omits `order`. Dates read + /// newest-first (`Desc`); text reads A→Z (`Asc`). This mirrors the + /// frontend's `defaultOrderFor` (see `frontend/src/lib/mangaSort.ts`) so a + /// bare `?sort=` link means the same thing to the UI and the API. + pub fn default_order(self) -> SortOrder { + match self { + SortField::Created | SortField::Updated => SortOrder::Desc, + SortField::Title | SortField::Author => SortOrder::Asc, + } + } +} + +/// Sort direction. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub enum SortOrder { Asc, #[default] @@ -148,15 +159,15 @@ const FILTER_WHERE: &str = r#" "#; /// Returns the page of mangas matching `query` plus the unfiltered total -/// count for the same filter. The trigram GIN indexes (see 0005_search.sql -/// and 0009_manga_metadata.sql) keep both queries cheap as the library -/// grows. +/// count for the same filter. The date/title sort columns are index-backed +/// (`mangas_created_at_idx`, `mangas_updated_at_idx`, `mangas_title_lower_idx`) +/// and the trigram GIN indexes (0005_search.sql, 0009_manga_metadata.sql) keep +/// the search filter cheap as the library grows. pub async fn list(pool: &PgPool, query: &ListQuery) -> AppResult<(Vec, i64)> { // Both `col` and `dir` are interpolated from hard-coded enums, never from - // request input, so this is not a SQL injection seam. `NULLS LAST` keeps - // authorless mangas out of the way in both directions, and the trailing - // `id` is a stable tie-break that keeps pagination deterministic across - // rows with equal sort keys. + // request input, so this is not a SQL injection seam. The trailing `id` is + // a stable tie-break that keeps pagination deterministic across rows with + // equal sort keys. let col = match query.sort { SortField::Created => "created_at", SortField::Updated => "updated_at", @@ -176,7 +187,14 @@ pub async fn list(pool: &PgPool, query: &ListQuery) -> AppResult<(Vec, i6 SortOrder::Asc => "ASC", SortOrder::Desc => "DESC", }; - let order_by = format!("{col} {dir} NULLS LAST, id"); + // Only the author key can be NULL (authorless mangas); keep those last in + // both directions. The date/title columns are NOT NULL, so they need no + // NULLS clause — and omitting it lets a reverse index scan satisfy `DESC`. + let nulls = match query.sort { + SortField::Author => " NULLS LAST", + _ => "", + }; + let order_by = format!("{col} {dir}{nulls}, id"); let search = query.search.as_deref(); let status = query.status.as_deref(); diff --git a/backend/tests/api_mangas.rs b/backend/tests/api_mangas.rs index 3c36591..2892ba1 100644 --- a/backend/tests/api_mangas.rs +++ b/backend/tests/api_mangas.rs @@ -281,6 +281,178 @@ async fn list_sort_author_orders_by_author_name_nulls_last(pool: PgPool) { assert_eq!(title_list(&body), vec!["Chi", "Akira", "Boku", "NoAuthor"]); } +#[sqlx::test(migrations = "./migrations")] +async fn list_sort_recent_alias_maps_to_created_desc(pool: PgPool) { + // `recent` was the pre-0.88 sort value (newest created first). It is kept + // as an alias for `created` so existing bots/scripts don't break. + let h = common::harness(pool.clone()); + let (_, cookie) = common::register_user(&h.app).await; + + for title in ["Alpha", "Bravo", "Charlie"] { + seed(&h.app, &cookie, title).await; + } + set_created_at(&pool, "Alpha", "2020-01-01T00:00:00Z").await; + set_created_at(&pool, "Bravo", "2023-01-01T00:00:00Z").await; + set_created_at(&pool, "Charlie", "2021-01-01T00:00:00Z").await; + + let resp = h + .app + .oneshot(common::get("/api/v1/mangas?sort=recent")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = common::body_json(resp).await; + // Same ordering as `sort=created&order=desc`: newest-created first. + assert_eq!(title_list(&body), vec!["Bravo", "Charlie", "Alpha"]); +} + +#[sqlx::test(migrations = "./migrations")] +async fn list_sort_field_defaults_direction_per_field(pool: PgPool) { + // When `order` is omitted, the API applies the field's natural direction: + // text fields ascend (A→Z), matching the frontend's `defaultOrderFor`, so + // a bare `?sort=title` link reads the same in the UI and over the wire. + let h = common::harness(pool); + let (_, cookie) = common::register_user(&h.app).await; + + for title in ["Vinland Saga", "Berserk", "One Piece"] { + seed(&h.app, &cookie, title).await; + } + + let resp = h + .app + .oneshot(common::get("/api/v1/mangas?sort=title")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = common::body_json(resp).await; + assert_eq!(title_list(&body), vec!["Berserk", "One Piece", "Vinland Saga"]); +} + +#[sqlx::test(migrations = "./migrations")] +async fn list_sort_author_default_is_ascending_nulls_last(pool: PgPool) { + // The other text field: a bare `?sort=author` must default to ascending and + // keep authorless mangas last (NULLS LAST), exercising the omitted-order + // path through the author subquery — not just `title`. + let h = common::harness(pool); + let (_, cookie) = common::register_user(&h.app).await; + + for (title, authors) in [ + ("Akira", json!(["Beta Author"])), + ("Boku", json!(["Alpha Author"])), + ("NoAuthor", json!([])), + ] { + let _ = h + .app + .clone() + .oneshot(common::post_multipart_with_cookie( + "/api/v1/mangas", + MultipartBuilder::new() + .add_json("metadata", json!({ "title": title, "authors": authors })), + &cookie, + )) + .await + .unwrap(); + } + + let resp = h + .app + .oneshot(common::get("/api/v1/mangas?sort=author")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = common::body_json(resp).await; + // Alpha Author (Boku) < Beta Author (Akira); authorless sorts last. + assert_eq!(title_list(&body), vec!["Boku", "Akira", "NoAuthor"]); +} + +#[sqlx::test(migrations = "./migrations")] +async fn list_rejects_invalid_sort_and_order_with_envelope(pool: PgPool) { + let h = common::harness(pool); + + // Unknown sort field → structured 422, not axum's plain-text 400. + let resp = h + .app + .clone() + .oneshot(common::get("/api/v1/mangas?sort=bogus")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["details"]["sort"], "invalid"); + + // Unknown order → same treatment. + let resp = h + .app + .oneshot(common::get("/api/v1/mangas?order=sideways")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["details"]["order"], "invalid"); +} + +/// Pin a manga's primary key to an explicit UUID. Safe only for freshly +/// seeded mangas with no chapters/authors/tags referencing them. +async fn set_id(pool: &PgPool, title: &str, id: &str) { + let uuid: uuid::Uuid = id.parse().unwrap(); + sqlx::query("UPDATE mangas SET id = $1 WHERE title = $2") + .bind(uuid) + .bind(title) + .execute(pool) + .await + .unwrap(); +} + +#[sqlx::test(migrations = "./migrations")] +async fn list_tie_break_orders_equal_keys_by_ascending_id(pool: PgPool) { + // The trailing `, id` in ORDER BY is what keeps pagination stable when the + // primary sort key ties. Pin three mangas to the SAME created_at and assign + // ids whose ascending order (Bravo < Charlie < Alpha) deliberately differs + // from insertion/physical order, so the expected ordering can ONLY come + // from the id tie-break. + // + // We sort by `created` on purpose: `mangas_created_at_idx` is `(created_at + // DESC)` with no id, so it does NOT encode the secondary order — unlike the + // default `updated` path, whose `mangas_updated_at_idx (updated_at DESC, + // id)` would mask a missing clause. Drop `, id` from the query and this + // test fails. + let h = common::harness(pool.clone()); + let (_, cookie) = common::register_user(&h.app).await; + + for title in ["Alpha", "Bravo", "Charlie"] { + seed(&h.app, &cookie, title).await; + set_created_at(&pool, title, "2022-01-01T00:00:00Z").await; + } + set_id(&pool, "Alpha", "33333333-3333-4333-8333-333333333333").await; + set_id(&pool, "Bravo", "11111111-1111-4111-8111-111111111111").await; + set_id(&pool, "Charlie", "22222222-2222-4222-8222-222222222222").await; + + let page = |limit: i64, offset: i64| { + let app = h.app.clone(); + async move { + let resp = app + .oneshot(common::get(&format!( + "/api/v1/mangas?sort=created&limit={limit}&offset={offset}" + ))) + .await + .unwrap(); + common::body_json(resp).await + } + }; + + // Single page: tied rows come back in ascending-id order. + let full = page(50, 0).await; + assert_eq!(title_list(&full), vec!["Bravo", "Charlie", "Alpha"]); + + // Paginating one row at a time reproduces that exact sequence — no overlap, + // no gap — which only holds because the id tie-break makes the order total. + let paged: Vec = vec![page(1, 0).await, page(1, 1).await, page(1, 2).await] + .iter() + .map(|b| b["items"][0]["title"].as_str().unwrap().to_string()) + .collect(); + assert_eq!(paged, vec!["Bravo", "Charlie", "Alpha"]); +} + #[sqlx::test(migrations = "./migrations")] async fn search_reflects_filtered_total(pool: PgPool) { let h = common::harness(pool); diff --git a/frontend/e2e/mobile-list-search.spec.ts b/frontend/e2e/mobile-list-search.spec.ts index 9e4ea2e..3638d11 100644 --- a/frontend/e2e/mobile-list-search.spec.ts +++ b/frontend/e2e/mobile-list-search.spec.ts @@ -185,5 +185,10 @@ test.describe('mobile catalog chrome', () => { await page.getByTestId('sort-order-mobile-desc').click(); await expect(page).toHaveURL(/order=desc/); expect(lastOrderParam).toBe('desc'); + + // The Done button gives an explicit dismissal affordance once the + // field + direction are set. + await page.getByTestId('sort-done').click(); + await expect(page.getByTestId('sort-sheet')).toBeHidden(); }); }); diff --git a/frontend/src/lib/components/SegmentedControl.svelte b/frontend/src/lib/components/SegmentedControl.svelte index 7c36015..a75571d 100644 --- a/frontend/src/lib/components/SegmentedControl.svelte +++ b/frontend/src/lib/components/SegmentedControl.svelte @@ -17,14 +17,69 @@ ariaLabel: string; testid?: string; } = $props(); + + // Roving-tabindex refs so arrow keys can move focus across the group. + let buttons = $state([]); + + function select(index: number) { + const opt = options[index]; + if (!opt) return; + if (opt.value !== value) onchange(opt.value); + // Selection follows focus, per the WAI-ARIA radiogroup pattern. + buttons[index]?.focus(); + } + + // ArrowLeft/Up and ArrowRight/Down move (and wrap) through the options; + // Home/End jump to the ends. Without this a control announced as a + // `radiogroup` would not respond to the keys AT users expect. + function onkeydown(event: KeyboardEvent) { + // Navigate relative to the focused button, not the selected `value`: + // this control is parent-controlled, so `value` only updates after a + // re-render — anchoring on focus keeps rapid keypresses from sticking. + // Falls back to `value` when focus is elsewhere (e.g. event delegated + // from outside a child, as in unit tests). + const focused = buttons.findIndex((b) => b === document.activeElement); + const current = focused >= 0 ? focused : options.findIndex((o) => o.value === value); + switch (event.key) { + case 'ArrowRight': + case 'ArrowDown': + event.preventDefault(); + select((current + 1) % options.length); + break; + case 'ArrowLeft': + case 'ArrowUp': + event.preventDefault(); + select((current - 1 + options.length) % options.length); + break; + case 'Home': + event.preventDefault(); + select(0); + break; + case 'End': + event.preventDefault(); + select(options.length - 1); + break; + } + } -
- {#each options as opt (opt.value)} + + +
+ {#each options as opt, i (opt.value)}
@@ -585,7 +572,8 @@ onClose={() => (sortSheetOpen = false)} testid="sort-sheet" > -
+

Sort by

+
{#each Object.entries(SORT_FIELD_LABELS) as [value, label] (value)} {/each}
+

Direction

+ {#snippet footer()} + + {/snippet} {#if loading} @@ -901,6 +902,26 @@ border-top: 1px solid var(--border); } + /* Section headings inside the mobile sort sheet ("Sort by" / "Direction") + so each group is visibly labelled, mirroring the desktop inline labels. */ + .sort-section-label { + margin: 0 0 var(--space-2); + font-size: var(--font-sm); + font-weight: var(--weight-medium); + color: var(--text-muted); + } + + .primary { + border: 1px solid var(--primary); + background: var(--primary); + color: var(--primary-contrast); + } + + .primary:hover { + background: var(--primary-hover); + border-color: var(--primary-hover); + } + .icon-btn { display: inline-flex; align-items: center;