diff --git a/CHANGELOG.md b/CHANGELOG.md index 347f8100b..5d5110637 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,17 @@ released versions carry their date on the heading. is a sentence that names the item, such as "The file sms-2.xml could not be read in full: …", where it was `error: sms-2.xml: This file could not be read in full: …` before. - +- 2026-10-05: **The server picks the order of the Messages list.** With no + `sort`, `GET /v1/messages` now puts the best match first when the search + has a free-text word to rank by, and the newest message first when it has + none. It used to list the oldest first. Each page says which order it + applied and which words it ranks by, in a new `search` key beside `items`, + and the web app reads both from there: the sort menu offers Relevance only + when words came back, and the list draws those words in bold. Nothing + changes on screen, except that a search the web app and the server once + read differently now shows the server's reading. Picking Relevance now + leaves `sort` out of the address, so the next search starts in the + server's order. - 2026-10-05: **A message's reply count is counted when it is read.** It is the number of replies Message Crate shows that quote the message, so a reply hidden as a duplicate no longer counts, and a reply that quotes a diff --git a/crates/server/server/src/db/conversation_messages.rs b/crates/server/server/src/db/conversation_messages.rs index 3926bed7f..29ab268df 100644 --- a/crates/server/server/src/db/conversation_messages.rs +++ b/crates/server/server/src/db/conversation_messages.rs @@ -189,18 +189,59 @@ pub enum MessageListSort { Relevance, } +impl MessageListSort { + /// The key as `sort=` spells it. + #[must_use] + pub const fn name(self) -> &'static str { + match self { + Self::Date => "date", + Self::Relevance => "relevance", + } + } +} + /// The Messages list's keys, as `sort=` spells them. pub const MESSAGE_LIST_SORT_KEYS: [(&str, MessageListSort); 2] = [ - ("date", MessageListSort::Date), - ("relevance", MessageListSort::Relevance), + (MessageListSort::Date.name(), MessageListSort::Date), + ( + MessageListSort::Relevance.name(), + MessageListSort::Relevance, + ), ]; -/// Oldest first, as [`DEFAULT_MESSAGE_SORT`] reads a conversation when `sort` -/// is absent. -pub const DEFAULT_MESSAGE_LIST_SORT: [SortKey; 1] = [SortKey { - key: MessageListSort::Date, - direction: Direction::Asc, -}]; +/// The order the Messages list applies when the request names no `sort`: +/// best match first when the query has a free-text word to rank by +/// (`ranked`), and newest first otherwise. A search with words is looking for +/// the messages that hold them, and one with only field words is browsing, +/// where the latest messages come first (#1538). +#[must_use] +pub fn default_message_list_sort(ranked: bool) -> [SortKey; 1] { + if ranked { + [SortKey { + key: MessageListSort::Relevance, + direction: Direction::Asc, + }] + } else { + [SortKey { + key: MessageListSort::Date, + direction: Direction::Desc, + }] + } +} + +/// `order` spelled as `sort=` takes it: `relevance`, `date`, `-date`, or +/// keys joined by commas, such as `relevance,-date`. +#[must_use] +pub fn message_list_sort_text(order: &[SortKey]) -> String { + order + .iter() + .map(|k| match k.direction { + Direction::Asc => k.key.name().to_string(), + Direction::Desc => format!("-{}", k.key.name()), + }) + .collect::>() + .join(",") +} /// The join a relevance order ranks by: every message the rank query /// matches, with its `bm25()`, keyed by message id. Its one `?` is the rank @@ -360,7 +401,7 @@ pub(crate) fn message_list_page_sql( )); }; from_sql.push_str(RANK_JOIN_SQL); - params.push(SqlParam::Text(rank_query.to_string())); + params.push(SqlParam::Text(rank_query)); } params.extend_from_slice(filter.params()); diff --git a/crates/server/server/src/messages_api.rs b/crates/server/server/src/messages_api.rs index 2ceda5299..27262b4da 100644 --- a/crates/server/server/src/messages_api.rs +++ b/crates/server/server/src/messages_api.rs @@ -9,15 +9,79 @@ use crate::extract::{Json, Path, Query}; use axum::extract::State; +use serde::Serialize; use crate::db::conversation_messages::{ - DEFAULT_MESSAGE_LIST_SORT, DEFAULT_MESSAGE_SORT, MESSAGE_LIST_SORT_KEYS, Message, - count_matching_messages, load_message_list_page, load_messages, + DEFAULT_MESSAGE_SORT, MESSAGE_LIST_SORT_KEYS, Message, count_matching_messages, + default_message_list_sort, load_message_list_page, load_messages, message_list_sort_text, }; use crate::db::sql::SqlParam; -use crate::paging::{ListRequest, Page, PageQuery}; +use crate::paging::{ListRequest, PageQuery}; +use crate::search::parse::TextTerm; use crate::server::{ApiError, AppState, FullAccess}; +/// One page of the Messages list and how the server read its search: the +/// four keys of every page, and `search`, the one key a page carries beside +/// them (`docs/architecture/http-api.md`, "Lists"). +// The four keys are written out rather than taken from `Page` with +// `#[serde(flatten)]`: utoipa describes a flattened field as an `allOf` of +// two schemas, which gives the page no `properties` of its own for the page +// rules (`openapi/document_rules.rs`) and the generated web types to read. +#[derive(Debug, Serialize, utoipa::ToSchema)] +pub(crate) struct ListMessagesResponse { + /// The rows on this page. + pub(crate) items: Vec, + /// Rows matching the query across every page. + pub(crate) total: u64, + /// Page size used. + pub(crate) limit: usize, + /// Page offset used. + pub(crate) offset: usize, + /// How the server read `q` and `sort`: the order it applied and the + /// free-text terms it ranks by. It describes the query, not the rows, so + /// every page of one query carries the same value. + pub(crate) search: MessageSearch, +} + +/// A Messages search as the server read it. +#[derive(Debug, Serialize, utoipa::ToSchema)] +pub(crate) struct MessageSearch { + /// The order the page is in, spelled as `sort` takes it: the `sort` the + /// request named, or with none, `relevance` when `terms` is not empty and + /// `-date` (newest first) when it is. + pub(crate) sort: String, + /// The free-text terms the search ranks by, in the order they were typed: + /// every word and quoted phrase of `q` that is not a field word and not + /// behind `-` or `not`, alone or in a negated group. Empty when `q` has + /// none, and then `relevance` is refused. + pub(crate) terms: Vec, +} + +/// One free-text term of a search: a word, or a quoted phrase. +#[derive(Debug, Serialize, utoipa::ToSchema)] +pub(crate) struct FreeTextTerm { + /// The word, or the phrase without its quotes, as typed. + pub(crate) text: String, + /// True when the word ended in `*` and matches any word it begins; the + /// `*` is not in `text`. Always false for a phrase. + pub(crate) prefix: bool, +} + +impl From<&TextTerm> for FreeTextTerm { + fn from(term: &TextTerm) -> Self { + match term { + TextTerm::Term { text, prefix } => Self { + text: text.clone(), + prefix: *prefix, + }, + TextTerm::Phrase(text) => Self { + text: text.clone(), + prefix: false, + }, + } + } +} + /// Compile a query against the Messages list of the search language. /// /// # Errors @@ -39,9 +103,14 @@ pub(crate) fn message_filter( })?) } -/// Messages matching `q`, oldest first unless `sort` says otherwise: the same -/// rows an Export Run with a `query` scope would hand over, behind a logged-in -/// session with the list defaults and the list's offset ceiling. +/// Messages matching `q`: the same rows an Export Run with a `query` scope +/// would hand over, behind a logged-in session with the list defaults and the +/// list's offset ceiling. +/// +/// With no `sort`, the best match comes first when `q` has a free-text word +/// to rank by, and the newest message first when it has none. The page's +/// `search` says which order it applied and which terms it ranks by, so a +/// client never parses `q` itself. /// /// `sort=relevance` puts the best match first, ranked by the full-text /// index's `bm25()` on the query's free-text words: the words not behind `-` @@ -58,10 +127,10 @@ pub(crate) fn message_filter( ("q" = Option, Query, description = "Search query in the Messages list's words; empty matches every message"), ("limit" = Option, Query, description = "Page size, default 40, max 500"), ("offset" = Option, Query, description = "Page offset, max 50000"), - ("sort" = Option, Query, description = "`date`, `-date` or `relevance` (best match first; needs a free-text word in `q`). Default `date`, oldest first.") + ("sort" = Option, Query, description = "`date`, `-date` or `relevance` (best match first; needs a free-text word in `q`). Default `relevance` when `q` has a free-text word, and `-date`, newest first, when it has none.") ), responses( - (status = 200, body = crate::paging::Page), + (status = 200, body = ListMessagesResponse), crate::problem::openapi::SearchQueryInvalid ) )] @@ -69,31 +138,46 @@ pub(crate) async fn list_messages( State(state): State, FullAccess(auth): FullAccess, Query(query): Query, -) -> Result>, ApiError> { +) -> Result, ApiError> { let mut conn = state.db.acquire().await?; + // No default here: which one applies depends on the query, which is + // compiled only after the sort has been checked. let list = ListRequest::read( &mut conn, auth.account_id, query, &MESSAGE_LIST_SORT_KEYS, - &DEFAULT_MESSAGE_LIST_SORT, + &[], ) .await?; let filter = message_filter(auth.account_id, &list.q, list.clock)?; + let order = if list.order.is_empty() { + default_message_list_sort(!filter.ranked_terms().is_empty()).to_vec() + } else { + list.order + }; let items = load_message_list_page( &mut conn, &filter, - &list.order, + &order, list.page.limit, list.page.offset, ) .await?; let total = count_matching_messages(&mut conn, &filter).await?; - Ok(Json(Page { + Ok(Json(ListMessagesResponse { items, total, limit: list.page.limit, offset: list.page.offset, + search: MessageSearch { + sort: message_list_sort_text(&order), + terms: filter + .ranked_terms() + .iter() + .map(FreeTextTerm::from) + .collect(), + }, })) } diff --git a/crates/server/server/src/messages_api/tests.rs b/crates/server/server/src/messages_api/tests.rs index f592772eb..22f4b3e4e 100644 --- a/crates/server/server/src/messages_api/tests.rs +++ b/crates/server/server/src/messages_api/tests.rs @@ -101,20 +101,22 @@ async fn the_messages_route_is_a_page_across_every_conversation() { assert_eq!(page["limit"], serde_json::json!(40)); assert_eq!(page["offset"], serde_json::json!(0)); assert_eq!(page["items"].as_array().unwrap().len(), 3); - // `docs/architecture/http-api.md`: a list is {items, total, limit, offset} and nothing else. + // `docs/architecture/http-api.md`, "Lists": a list is {items, total, + // limit, offset}, and the Messages list adds `search`, its one exception. let keys: Vec<&str> = page .as_object() .unwrap() .keys() .map(String::as_str) .collect(); - assert_eq!(keys, ["items", "limit", "offset", "total"]); + assert_eq!(keys, ["items", "limit", "offset", "search", "total"]); } #[tokio::test] async fn a_page_across_two_conversations_names_each_conversations_own_participants() { let (fixture, alice, direct, group) = seeded().await; - let page: serde_json::Value = get_json(&fixture.state, "/v1/messages", &alice.token).await; + let page: serde_json::Value = + get_json(&fixture.state, "/v1/messages?sort=date", &alice.token).await; let handles: Vec<(i64, &str)> = page["items"] .as_array() @@ -1970,6 +1972,102 @@ async fn relevance_ranks_by_the_positive_words_only() { assert_eq!(texts(&page)[0], "the dentist moved it", "{page}"); } +/// With no `sort`, a search with a free-text word puts the best match first +/// and says so, and a search with only field words lists the newest message +/// first and says so (#1538). The client reads the order back rather than +/// parsing `q` to guess it. +#[tokio::test] +async fn with_no_sort_the_server_picks_the_order_and_reports_it() { + let (fixture, alice) = seeded_for_relevance().await; + let page: serde_json::Value = + get_json(&fixture.state, "/v1/messages?q=dentist", &alice.token).await; + assert_eq!( + texts(&page), + [ + "dentist dentist dentist", + "the dentist moved it", + "after work I will call the office of the dentist about next week", + ], + "{page}" + ); + assert_eq!( + page["search"], + serde_json::json!({"sort": "relevance", "terms": [{"text": "dentist", "prefix": false}]}) + ); + + let page: serde_json::Value = + get_json(&fixture.state, "/v1/messages?q=date%3A2024", &alice.token).await; + assert_eq!( + texts(&page), + [ + "nothing to see", + "the dentist moved it", + "after work I will call the office of the dentist about next week", + "dentist dentist dentist", + ], + "{page}" + ); + assert_eq!( + page["search"], + serde_json::json!({"sort": "-date", "terms": []}) + ); +} + +/// A `sort` the request names is applied as it is, and reported in the +/// spelling `sort` takes, keys and signs included. +#[tokio::test] +async fn a_named_sort_is_applied_and_reported() { + let (fixture, alice) = seeded_for_relevance().await; + for (sort, reported) in [ + ("date", "date"), + ("-DATE", "-date"), + ("relevance,date", "relevance,date"), + ] { + let page: serde_json::Value = get_json( + &fixture.state, + &format!("/v1/messages?q=dentist&sort={sort}"), + &alice.token, + ) + .await; + assert_eq!( + page["search"]["sort"], + serde_json::json!(reported), + "{sort}: {page}" + ); + } + let page: serde_json::Value = get_json( + &fixture.state, + "/v1/messages?q=dentist&sort=date", + &alice.token, + ) + .await; + assert_eq!(texts(&page)[0], "dentist dentist dentist", "{page}"); +} + +/// The terms a search ranks by are the free-text words and phrases a match +/// must or may have, in the order typed: a word behind `-` or `not`, a word +/// in a negated group, and a field word's value are none of them. A prefix +/// keeps its `*` as a flag, and a phrase comes back without its quotes. +#[tokio::test] +async fn the_page_names_the_terms_the_search_ranks_by() { + let (fixture, alice) = seeded_for_relevance().await; + // dentist -office not moved -("next week" or call) body:work date:2024 + // "the dentist" or den* + let q = "dentist%20-office%20not%20moved%20-(%22next%20week%22%20or%20call)%20body%3Awork%20date%3A2024%20%22the%20dentist%22%20or%20den*"; + let page: serde_json::Value = + get_json(&fixture.state, &format!("/v1/messages?q={q}"), &alice.token).await; + assert_eq!( + page["search"]["terms"], + serde_json::json!([ + {"text": "dentist", "prefix": false}, + {"text": "the dentist", "prefix": false}, + {"text": "den", "prefix": true}, + ]), + "{page}" + ); + assert_eq!(page["search"]["sort"], serde_json::json!("relevance")); +} + /// Relevance has one direction, best match first: `-relevance` would list the /// unranked messages first and then the worst match, which nobody asks for. #[tokio::test] diff --git a/crates/server/server/src/openapi/document_rules.rs b/crates/server/server/src/openapi/document_rules.rs index 5cb11cb74..668c5dc98 100644 --- a/crates/server/server/src/openapi/document_rules.rs +++ b/crates/server/server/src/openapi/document_rules.rs @@ -46,6 +46,32 @@ const MULTIPART_PART: &str = "/v1/assets/{sha256}/uploads/{upload_id}/parts/{par /// The four keys of every page. const PAGE_KEYS: [&str; 4] = ["items", "total", "limit", "offset"]; +/// The one page with a key beside the four, and the one route that answers +/// it: `GET /v1/messages` says how it read its search in `search` +/// (`docs/architecture/http-api.md`, "Lists"). Named here, route and schema +/// both. Any other schema with the four keys is a page too, whatever its +/// name ([`page_schemas`]), so a second page with a fifth key fails the +/// rules. +struct PageException { + method: &'static str, + path: &'static str, + schema: &'static str, + key: &'static str, +} + +const PAGE_EXCEPTION: PageException = PageException { + method: "get", + path: "/v1/messages", + schema: "ListMessagesResponse", + key: "search", +}; + +impl PageException { + fn is(&self, op: &Operation) -> bool { + op.method == self.method && op.path == self.path + } +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn every_operation_keeps_the_rules_the_document_can_show() { let doc: Value = serde_json::from_str(&dump_openapi_json()).unwrap(); @@ -185,6 +211,24 @@ fn read_rules(doc: &Value, op: &Operation, spec: &Value) -> Vec { .map(|field| format!("{field} is optional in a success answer")), ); + // The exception is checked by its route as well as its schema: the + // route must answer it, and no other route may. + let answered = schema_named(&spec["responses"]["200"]["content"]["application/json"]["schema"]); + if PAGE_EXCEPTION.is(op) && answered != Some(PAGE_EXCEPTION.schema) { + broken.push(format!( + "the page with a fifth key is {}, and this route answers {answered:?}", + PAGE_EXCEPTION.schema + )); + } + if !PAGE_EXCEPTION.is(op) && answered == Some(PAGE_EXCEPTION.schema) { + broken.push(format!( + "answers {}, the page only {} {} may answer", + PAGE_EXCEPTION.schema, + PAGE_EXCEPTION.method.to_uppercase(), + PAGE_EXCEPTION.path + )); + } + for page in page_schemas(doc, spec) { let required: BTreeSet<&str> = page["required"] .as_array() @@ -192,11 +236,25 @@ fn read_rules(doc: &Value, op: &Operation, spec: &Value) -> Vec { .flatten() .filter_map(Value::as_str) .collect(); - for key in PAGE_KEYS { + let properties: BTreeSet<&str> = page["properties"] + .as_object() + .into_iter() + .flat_map(|p| p.keys().map(String::as_str)) + .collect(); + let mut keys: BTreeSet<&str> = PAGE_KEYS.into_iter().collect(); + if PAGE_EXCEPTION.is(op) { + keys.insert(PAGE_EXCEPTION.key); + } + for key in &keys { if !required.contains(key) { broken.push(format!("the page it answers has no required {key}")); } } + for key in properties.difference(&keys) { + broken.push(format!( + "the page it answers has {key} beside its four keys (\"Lists\")" + )); + } // A POST that reads the rows its body names answers the whole body // as one page, and takes no paging ("Lists"). if op.method == "get" { @@ -887,7 +945,9 @@ fn is_kebab(segment: &str) -> bool { /// The page schemas a `200` answers: the page it names, or each page of a /// choice between pages, as an account's history answers the account in -/// full and the owner without content. Empty when it answers no page. +/// full and the owner without content. A schema is a page by its name or by +/// its shape ([`is_page`]), so a page under a name that does not start with +/// `Page_` is still held to the page rules. Empty when it answers no page. fn page_schemas<'d>(doc: &'d Value, spec: &Value) -> Vec<&'d Value> { let schemas = &doc["components"]["schemas"]; let Some(name) = @@ -895,7 +955,7 @@ fn page_schemas<'d>(doc: &'d Value, spec: &Value) -> Vec<&'d Value> { else { return Vec::new(); }; - if name.starts_with("Page_") { + if is_page(name, &schemas[name]) { return vec![&schemas[name]]; } let choices: Vec<&str> = schemas[name]["oneOf"] @@ -904,13 +964,23 @@ fn page_schemas<'d>(doc: &'d Value, spec: &Value) -> Vec<&'d Value> { .flatten() .filter_map(schema_named) .collect(); - if !choices.is_empty() && choices.iter().all(|c| c.starts_with("Page_")) { + if !choices.is_empty() && choices.iter().all(|c| is_page(c, &schemas[*c])) { choices.into_iter().map(|c| &schemas[c]).collect() } else { Vec::new() } } +/// Whether the schema `name` is a page: `Page`'s own schemas by their +/// `Page_` name, so one that lost a key still fails the page rules, and any +/// other schema with all four [`PAGE_KEYS`] among its properties. +fn is_page(name: &str, schema: &Value) -> bool { + name.starts_with("Page_") + || PAGE_KEYS + .iter() + .all(|key| schema["properties"].get(*key).is_some()) +} + /// What the operation says about its `offset`. fn offset_description(spec: &Value) -> &str { spec["parameters"] diff --git a/crates/server/server/src/search/emit.rs b/crates/server/server/src/search/emit.rs index f6880f01e..110db2cfc 100644 --- a/crates/server/server/src/search/emit.rs +++ b/crates/server/server/src/search/emit.rs @@ -56,30 +56,30 @@ pub(crate) fn compile( account_id: i64, zone: chrono_tz::Tz, ) -> Result { - let rank_query = match (list, expr) { - (ListKind::Messages, Some(expr)) => fts::rank_query(&expr.positive_text_terms()), - _ => None, + let ranked_terms: Vec = match (list, expr) { + (ListKind::Messages, Some(expr)) => { + expr.positive_text_terms().into_iter().cloned().collect() + } + _ => Vec::new(), }; let (where_sql, params) = compile_where(list, expr, account_id, zone, true)?; + let mut filter = Filter { + where_sql, + params, + ranked_terms, + final_text: None, + earlier_version_match: None, + }; // Only a free-text word that is not negated can find a message by an // earlier version alone: a negated one only leaves messages out, and // reading earlier versions too leaves out more, never adds. - let final_text = match rank_query { - Some(_) => Some(compile_where(list, expr, account_id, zone, false)?), - None => None, - }; - let earlier_version_match = rank_query.as_deref().map(|query| { + if let Some(query) = filter.rank_query() { + filter.final_text = Some(compile_where(list, expr, account_id, zone, false)?); let mut out = Sql::default(); - fts::version_ids_matching(&mut out, query); - (out.text, out.params) - }); - Ok(Filter { - where_sql, - params, - rank_query, - final_text, - earlier_version_match, - }) + fts::version_ids_matching(&mut out, &query); + filter.earlier_version_match = Some((out.text, out.params)); + } + Ok(filter) } /// The WHERE fragment and its values for `expr` on `list`, its free-text diff --git a/crates/server/server/src/search/mod.rs b/crates/server/server/src/search/mod.rs index fd2232488..b78475050 100644 --- a/crates/server/server/src/search/mod.rs +++ b/crates/server/server/src/search/mod.rs @@ -87,7 +87,7 @@ pub fn today_in(zone: chrono_tz::Tz) -> NaiveDate { pub struct Filter { where_sql: String, params: Vec, - rank_query: Option, + ranked_terms: Vec, final_text: Option<(String, Vec)>, earlier_version_match: Option<(String, Vec)>, } @@ -107,8 +107,16 @@ impl Filter { /// The full-text query a Messages search ranks by: its free-text words /// not under a negation, joined by `OR`, for `bm25()`. `None` on the /// other lists, and for a query with no such word. - pub fn rank_query(&self) -> Option<&str> { - self.rank_query.as_deref() + pub fn rank_query(&self) -> Option { + fts::rank_query(&self.ranked_terms.iter().collect::>()) + } + + /// The free-text terms [`Self::rank_query`] is built from, in the order + /// they were typed: every one not behind `-` or `not`, alone or in a + /// negated group (`Expr::positive_text_terms`). Empty when there is + /// nothing to rank by, and so `rank_query` is `None`. + pub(crate) fn ranked_terms(&self) -> &[parse::TextTerm] { + &self.ranked_terms } /// A `SELECT` of the ids of the earlier versions that hold one of the @@ -181,7 +189,7 @@ pub fn compile_messages_of_conversations(req: CompileRequest<'_>) -> Result setParam(MESSAGE_SORT_PARAM, messageSortParam(next))} + onSortPick={(next) => setParam(MESSAGE_SORT_PARAM, pickedSortParam(next) ?? "")} selectedId={openedAt(searchParams)} onSelect={(message) => // `q` stays what the person typed, and the tag rides beside it, diff --git a/web/src/lib/freeTextTerms.test.ts b/web/src/lib/freeTextTerms.test.ts deleted file mode 100644 index 6e3ce729d..000000000 --- a/web/src/lib/freeTextTerms.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { freeTextTerms, hasFreeText } from "./freeTextTerms"; - -describe("freeTextTerms", () => { - it("reads plain words and leaves field words out", () => { - expect(freeTextTerms("from:Alice photo")).toEqual([{ text: "photo", prefix: false }]); - expect(freeTextTerms('from:"Alice Smith" date:2024')).toEqual([]); - }); - - it("reads a quoted phrase as one term, with a doubled quote as one quote", () => { - expect(freeTextTerms('"see you ""there"""')).toEqual([ - { text: 'see you "there"', prefix: false }, - ]); - }); - - it("reads a trailing star as a prefix", () => { - expect(freeTextTerms("avoc*")).toEqual([{ text: "avoc", prefix: true }]); - }); - - it("leaves out a word behind a minus or not, and every word in a negated group", () => { - expect(freeTextTerms("dentist -office")).toEqual([{ text: "dentist", prefix: false }]); - expect(freeTextTerms("not dentist")).toEqual([]); - expect(freeTextTerms("-(toast or guacamole) avocado")).toEqual([ - { text: "avocado", prefix: false }, - ]); - expect(freeTextTerms("not (toast guacamole) avocado")).toEqual([ - { text: "avocado", prefix: false }, - ]); - }); - - it("reads the words on both sides of or and and, and not the operators", () => { - expect(freeTextTerms("moved OR dentist and (toast)")).toEqual([ - { text: "moved", prefix: false }, - { text: "dentist", prefix: false }, - { text: "toast", prefix: false }, - ]); - }); - - it("reads a word with a colon that is not a field word as text", () => { - expect(freeTextTerms("12:30 https://example.com")).toEqual([ - { text: "12:30", prefix: false }, - { text: "https://example.com", prefix: false }, - ]); - }); -}); - -describe("hasFreeText", () => { - it("is true only when a positive free-text word is left", () => { - expect(hasFreeText("from:Alice photo")).toBe(true); - expect(hasFreeText("from:Alice date:2024")).toBe(false); - expect(hasFreeText("-photo")).toBe(false); - expect(hasFreeText("")).toBe(false); - }); -}); diff --git a/web/src/lib/freeTextTerms.ts b/web/src/lib/freeTextTerms.ts deleted file mode 100644 index 21012a5c8..000000000 --- a/web/src/lib/freeTextTerms.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { searchTokens } from "./searchQuery"; - -/** One free-text term of a query: a word or a quoted phrase, and whether it ended in `*`. */ -export type FreeTextTerm = { text: string; prefix: boolean }; - -/** `word:` at the start of a token, as the server's lexer reads a field word. */ -const FIELD_HEAD = /^[A-Za-z][A-Za-z-]*:(?!\/)/; - -/** - * The free-text terms a match must or may have, in the order they were - * typed: every word and quoted phrase that is not a `word:value`, not an - * operator (`or`, `and`, `not`), and not behind `-` or `not`, alone or in a - * negated group. - * - * The server ranks a Messages search by these same terms - * (`Expr::positive_text_terms` in `crates/server/server/src/search/parse.rs`), - * so a query this finds none in is one the server will not sort by - * relevance. The Messages list draws them in bold. - */ -export function freeTextTerms(query: string): FreeTextTerm[] { - const terms: FreeTextTerm[] = []; - /** Whether each open group is negated, innermost last. */ - const groups: boolean[] = []; - let pendingNot = false; - for (const token of searchTokens(query)) { - const raw = query.slice(token.start, token.end); - const inNegated = groups.at(-1) ?? false; - if (token.kind === "close") { - groups.pop(); - continue; - } - const minus = raw.startsWith("-") && raw.length > 1; - const negated = inNegated || pendingNot || minus; - if (token.kind === "open") { - groups.push(negated); - pendingNot = false; - continue; - } - const lower = raw.toLowerCase(); - if (lower === "or" || lower === "and") continue; - if (lower === "not") { - pendingNot = true; - continue; - } - pendingNot = false; - const body = minus ? raw.slice(1) : raw; - if (negated || FIELD_HEAD.test(body)) continue; - const term = readTerm(body); - if (term) terms.push(term); - } - return terms; -} - -/** True when `query` has a free-text term to rank by. */ -export function hasFreeText(query: string): boolean { - return freeTextTerms(query).length > 0; -} - -/** One text token as a term: a quoted phrase unquoted, or a word with its `*` read off. */ -function readTerm(body: string): FreeTextTerm | null { - if (body.startsWith('"')) { - const inner = body.endsWith('"') && body.length > 1 ? body.slice(1, -1) : body.slice(1); - const text = inner.replace(/""/g, '"').trim(); - return text ? { text, prefix: false } : null; - } - if (body.endsWith("*") && body.length > 1) return { text: body.slice(0, -1), prefix: true }; - return body ? { text: body, prefix: false } : null; -} diff --git a/web/src/lib/messageMatch.ts b/web/src/lib/messageMatch.ts index ee60f9989..e4c21775e 100644 --- a/web/src/lib/messageMatch.ts +++ b/web/src/lib/messageMatch.ts @@ -1,4 +1,4 @@ -import type { FreeTextTerm } from "./freeTextTerms"; +import type { FreeTextTerm } from "./types"; /** A matched span of a string: start inclusive, end exclusive, in UTF-16 units. */ export type MatchRange = [number, number]; diff --git a/web/src/lib/messageSearchSort.test.ts b/web/src/lib/messageSearchSort.test.ts index 2c6f76719..df4385959 100644 --- a/web/src/lib/messageSearchSort.test.ts +++ b/web/src/lib/messageSearchSort.test.ts @@ -1,23 +1,5 @@ import { describe, expect, it } from "vitest"; -import { effectiveMessageSort, messageSortParam } from "./messageSearchSort"; - -describe("effectiveMessageSort", () => { - it("is Relevance by default when the query has a free-text word, and Date newest first otherwise", () => { - expect(effectiveMessageSort(null, true)).toEqual({ sort: "relevance", order: "desc" }); - expect(effectiveMessageSort(null, false)).toEqual({ sort: "date", order: "desc" }); - }); - - it("keeps the person's pick, except Relevance with nothing to rank by", () => { - expect(effectiveMessageSort({ sort: "date", order: "asc" }, true)).toEqual({ - sort: "date", - order: "asc", - }); - expect(effectiveMessageSort({ sort: "relevance", order: "desc" }, false)).toEqual({ - sort: "date", - order: "desc", - }); - }); -}); +import { messageSortFromParam, messageSortParam, pickedSortParam } from "./messageSearchSort"; describe("messageSortParam", () => { it("spells the sort as the route takes it", () => { @@ -27,3 +9,22 @@ describe("messageSortParam", () => { expect(messageSortParam({ sort: "date", order: "desc" })).toBe("-date"); }); }); + +describe("messageSortFromParam", () => { + it("reads the order the server reports, by its first key", () => { + expect(messageSortFromParam("relevance")).toEqual({ sort: "relevance", order: "desc" }); + expect(messageSortFromParam("date")).toEqual({ sort: "date", order: "asc" }); + expect(messageSortFromParam("-date")).toEqual({ sort: "date", order: "desc" }); + expect(messageSortFromParam("relevance,-date")).toEqual({ sort: "relevance", order: "desc" }); + expect(messageSortFromParam("colour")).toBeNull(); + expect(messageSortFromParam(null)).toBeNull(); + }); +}); + +describe("pickedSortParam", () => { + it("keeps a Date order, and nothing for Relevance, which is the server's own order", () => { + expect(pickedSortParam({ sort: "date", order: "asc" })).toBe("date"); + expect(pickedSortParam({ sort: "date", order: "desc" })).toBe("-date"); + expect(pickedSortParam({ sort: "relevance", order: "desc" })).toBeNull(); + }); +}); diff --git a/web/src/lib/messageSearchSort.ts b/web/src/lib/messageSearchSort.ts index dd635df19..7321646e1 100644 --- a/web/src/lib/messageSearchSort.ts +++ b/web/src/lib/messageSearchSort.ts @@ -7,19 +7,6 @@ export type MessageSearchSortKey = "relevance" | "date"; /** One choice in the Messages list's sort menu. Relevance has no order: best match first. */ export type MessageSearchSort = { sort: MessageSearchSortKey; order: SortOrder }; -/** - * The order the Messages list shows: the one the person picked, unless it is - * Relevance and the query has no free-text word to rank by. With no pick, - * Relevance when there is a word to rank by, and otherwise Date, newest first. - */ -export function effectiveMessageSort( - picked: MessageSearchSort | null, - rankable: boolean, -): MessageSearchSort { - if (picked && (picked.sort === "date" || rankable)) return picked; - return rankable ? { sort: "relevance", order: "desc" } : { sort: "date", order: "desc" }; -} - /** A `sort` value `GET /v1/messages` takes. */ export type MessageSortParam = NonNullable; @@ -27,21 +14,39 @@ export type MessageSortParam = NonNullable; * The `sort` value for each choice, read both ways: Relevance has one order, * and Date one value per order. */ -const SORT_PARAMS: Readonly> = { +const SORT_PARAMS = { relevance: "relevance", asc: "date", desc: "-date", -}; +} as const satisfies Readonly>; /** The `sort` parameter `GET /v1/messages` takes for `s`. */ export function messageSortParam(s: MessageSearchSort): MessageSortParam { return SORT_PARAMS[s.sort === "relevance" ? "relevance" : s.order]; } -/** The choice a `sort` value stands for, or null for a value it is not. */ +/** + * The choice a `sort` value stands for, or null for a value it is not. A + * value of several keys, such as `relevance,-date`, stands for its first, + * which decides the order. + */ export function messageSortFromParam(param: string | null): MessageSearchSort | null { - if (param === SORT_PARAMS.relevance) return { sort: "relevance", order: "desc" }; - if (param === SORT_PARAMS.asc) return { sort: "date", order: "asc" }; - if (param === SORT_PARAMS.desc) return { sort: "date", order: "desc" }; + const first = param?.split(",")[0]?.trim() ?? null; + if (first === SORT_PARAMS.relevance) return { sort: "relevance", order: "desc" }; + if (first === SORT_PARAMS.asc) return { sort: "date", order: "asc" }; + if (first === SORT_PARAMS.desc) return { sort: "date", order: "desc" }; return null; } + +/** + * What the person's pick in the sort menu keeps: a Date order as its `sort` + * value, or nothing for Relevance. + * + * Relevance is never kept as a pick. The menu offers it only for a search the + * server already ranks when no `sort` is sent, so picking it means "the + * server's order". A kept `relevance` would follow the person to their next + * search, and the server refuses it for a search with no free-text word. + */ +export function pickedSortParam(s: MessageSearchSort): (typeof SORT_PARAMS)[SortOrder] | null { + return s.sort === "relevance" ? null : SORT_PARAMS[s.order]; +} diff --git a/web/src/lib/queryKeys.ts b/web/src/lib/queryKeys.ts index 52ec00fee..7ae776650 100644 --- a/web/src/lib/queryKeys.ts +++ b/web/src/lib/queryKeys.ts @@ -82,7 +82,8 @@ export const keys = { }, /** * The Messages list: the messages a search matches across every - * conversation, `GET /v1/messages`, one entry per query and sort. + * conversation, `GET /v1/messages`, one entry per query and sort. The + * sort is `""` when the person picked none and the server picks it. */ messages: { all: ["messages"] as const, diff --git a/web/src/lib/resultsView.test.ts b/web/src/lib/resultsView.test.ts index f425440e6..81bc4eb5d 100644 --- a/web/src/lib/resultsView.test.ts +++ b/web/src/lib/resultsView.test.ts @@ -18,11 +18,8 @@ describe("resultsView", () => { }); describe("pickedMessageSort", () => { - it("reads the route's spelling, and nothing else", () => { - expect(pickedMessageSort(params("sort=relevance"))).toEqual({ - sort: "relevance", - order: "desc", - }); + it("reads a Date order in the route's spelling, and nothing else", () => { + expect(pickedMessageSort(params("sort=relevance"))).toBeNull(); expect(pickedMessageSort(params("sort=date"))).toEqual({ sort: "date", order: "asc" }); expect(pickedMessageSort(params("sort=-date"))).toEqual({ sort: "date", order: "desc" }); expect(pickedMessageSort(params("sort=colour"))).toBeNull(); diff --git a/web/src/lib/resultsView.ts b/web/src/lib/resultsView.ts index 8fbb12bb7..ad52cd74a 100644 --- a/web/src/lib/resultsView.ts +++ b/web/src/lib/resultsView.ts @@ -47,9 +47,15 @@ export function resultsView(params: URLSearchParams): ResultsView { return params.get(VIEW_PARAM) === "messages" ? "messages" : "conversations"; } -/** The sort the person picked in the Messages list, or null for the default. */ +/** + * The sort the person picked in the Messages list, or null for the server's + * own order. Only a Date order is a pick (`pickedSortParam`), so `relevance` + * in the address reads as no pick, and the server ranks the search when it + * has a free-text word to rank by. + */ export function pickedMessageSort(params: URLSearchParams): MessageSearchSort | null { - return messageSortFromParam(params.get(MESSAGE_SORT_PARAM)); + const picked = messageSortFromParam(params.get(MESSAGE_SORT_PARAM)); + return picked?.sort === "date" ? picked : null; } /** The Message Tag `params` lists by on `/messages/:id`, or null for none. */ diff --git a/web/src/lib/routeQuery.ts b/web/src/lib/routeQuery.ts index a76414a59..34a734eb3 100644 --- a/web/src/lib/routeQuery.ts +++ b/web/src/lib/routeQuery.ts @@ -161,17 +161,26 @@ export type OffsetPage = { */ const NO_PAGES: readonly unknown[] = []; -/** How a screen loads one page. */ -export type PagedFetchPage = (args: { +/** + * How a screen loads one page. A route that answers more than the rows and + * the total, as the Messages list answers its `search`, names the rest as `E`. + */ +export type PagedFetchPage = (args: { limit: number; offset: number; signal: AbortSignal; -}) => Promise>; +}) => Promise & E>; /** What a long list needs to render itself while it fills. */ -export type PagedListResult = { +export type PagedListResult = { items: T[]; total: number; + /** + * The latest page the server answered, or null before the first. A screen + * reads from it what the route says beside the rows, such as the order the + * Messages list applied. + */ + lastPage: (OffsetPage & E) | null; /** No page has arrived yet: the list has nothing to show. */ loading: boolean; /** The first page is being fetched again behind rows already on screen. */ @@ -222,9 +231,9 @@ function distinctRows(pages: readonly OffsetP * and another removed between two fetches leaves the total the same, and a * row skipped that way stays missing until the list is next fetched. */ -export function useRoutePagedList( +export function useRoutePagedList( key: RouteQueryKey, - fetchPage: PagedFetchPage, + fetchPage: PagedFetchPage, opts?: { firstPageSize?: number; fillPageSize?: number; @@ -236,7 +245,7 @@ export function useRoutePagedList( /** False holds the list back, as `enabled` does on `useQuery`. */ enabled?: boolean; }, -): PagedListResult { +): PagedListResult { const account = useAccountScope(); const firstPageSize = opts?.firstPageSize ?? PAGE_SIZE_FIRST; const fillPageSize = opts?.fillPageSize ?? PAGE_SIZE_FILL; @@ -249,13 +258,8 @@ export function useRoutePagedList( const keyText = JSON.stringify(queryKey); const largePagesFor = useRef(null); - const query = useInfiniteQuery< - OffsetPage, - Error, - InfiniteData>, - unknown[], - number - >({ + type P = OffsetPage & E; + const query = useInfiniteQuery, unknown[], number>({ queryKey, enabled: opts?.enabled ?? true, initialPageParam: 0, @@ -277,7 +281,7 @@ export function useRoutePagedList( }, }); - const pages = query.data?.pages ?? (NO_PAGES as OffsetPage[]); + const pages = query.data?.pages ?? (NO_PAGES as P[]); // A new array every render defeats every memo downstream (the tag menu and // its effect included), so this is the one place that must not recompute // unless the query actually produced new pages. @@ -294,6 +298,7 @@ export function useRoutePagedList( return { items, total: pages[pages.length - 1]?.total ?? 0, + lastPage: pages[pages.length - 1] ?? null, loading: query.isPending, // A refetch of what is already on screen, as opposed to a first load or a // page being appended. diff --git a/web/src/lib/serverApi.ts b/web/src/lib/serverApi.ts index b763538dc..f2c0a9618 100644 --- a/web/src/lib/serverApi.ts +++ b/web/src/lib/serverApi.ts @@ -541,8 +541,10 @@ export type MessagesListParams = { offset?: number; limit?: number; /** - * `date` (oldest first, the default), `-date`, or `relevance`: best match - * first, which needs a free-text word in `q` (`hasFreeText`). + * `date` (oldest first), `-date`, or `relevance`: best match first, which + * needs a free-text word in `q`. Left out, the server picks: `relevance` + * when `q` has a free-text word and `-date` when it has none, and the + * page's `search` says which it applied. */ sort?: "date" | "-date" | "relevance"; }; @@ -555,8 +557,11 @@ export type MessagesListParams = { export function listMessages( params: MessagesListParams, opts?: RequestOptions, -): Promise { - return apiClient.get(withQuery("/v1/messages", query(params)), opts); +): Promise { + return apiClient.get( + withQuery("/v1/messages", query(params)), + opts, + ); } /** Every source a conversation's messages came from. */ diff --git a/web/src/lib/serverApi.types.ts b/web/src/lib/serverApi.types.ts index 9bb8d9cea..15ae352f0 100644 --- a/web/src/lib/serverApi.types.ts +++ b/web/src/lib/serverApi.types.ts @@ -1192,8 +1192,13 @@ export interface paths { cookie?: never; }; /** - * Messages matching `q`, oldest first unless `sort` says otherwise: the same rows an Export Run with a `query` scope would hand over, behind a logged-in session with the list defaults and the list's offset ceiling. - * @description `sort=relevance` puts the best match first, ranked by the full-text + * Messages matching `q`: the same rows an Export Run with a `query` scope would hand over, behind a logged-in session with the list defaults and the list's offset ceiling. + * @description With no `sort`, the best match comes first when `q` has a free-text word + * to rank by, and the newest message first when it has none. The page's + * `search` says which order it applied and which terms it ranks by, so a + * client never parses `q` itself. + * + * `sort=relevance` puts the best match first, ranked by the full-text * index's `bm25()` on the query's free-text words: the words not behind `-` * or `not`. A query with no such word has nothing to rank by, and * `relevance` is then `validation-failed`. Ties, and a message found only by @@ -2792,6 +2797,16 @@ export interface components { /** @description The spelling, without the colon. */ word: string; }; + /** @description One free-text term of a search: a word, or a quoted phrase. */ + FreeTextTerm: { + /** + * @description True when the word ended in `*` and matches any word it begins; the + * `*` is not in `text`. Always false for a phrase. + */ + prefix: boolean; + /** @description The word, or the phrase without its quotes, as typed. */ + text: string; + }; /** * @description Which contacts `POST /v1/contacts/address-book` writes: the Contacts * list's search and its checked rows. @@ -3208,6 +3223,30 @@ export interface components { /** @description The most lines this page could hold. */ limit: number; }; + /** + * @description One page of the Messages list and how the server read its search: the + * four keys of every page, and `search`, the one key a page carries beside + * them (`docs/architecture/http-api.md`, "Lists"). + */ + ListMessagesResponse: { + /** @description The rows on this page. */ + items: components["schemas"]["Message"][]; + /** @description Page size used. */ + limit: number; + /** @description Page offset used. */ + offset: number; + /** + * @description How the server read `q` and `sort`: the order it applied and the + * free-text terms it ranks by. It describes the query, not the rows, so + * every page of one query carries the same value. + */ + search: components["schemas"]["MessageSearch"]; + /** + * Format: int64 + * @description Rows matching the query across every page. + */ + total: number; + }; /** @description Body for `POST /v1/contacts/unmatched-identities`. */ ListUnmatchedIdentitiesRequest: { /** @description Raw identifiers — phone numbers, emails — as they appear in an export. */ @@ -3412,6 +3451,22 @@ export interface components { /** @description Participants of the conversation. */ participants: components["schemas"]["Participant"][]; }; + /** @description A Messages search as the server read it. */ + MessageSearch: { + /** + * @description The order the page is in, spelled as `sort` takes it: the `sort` the + * request named, or with none, `relevance` when `terms` is not empty and + * `-date` (newest first) when it is. + */ + sort: string; + /** + * @description The free-text terms the search ranks by, in the order they were typed: + * every word and quoted phrase of `q` that is not a field word and not + * behind `-` or `not`, alone or in a negated group. Empty when `q` has + * none, and then `relevance` is refused. + */ + terms: components["schemas"]["FreeTextTerm"][]; + }; /** @description One Contact Group or Message Tag: its id and name. */ NamedSet: { /** @@ -11911,7 +11966,7 @@ export interface operations { limit?: number; /** @description Page offset, max 50000 */ offset?: number; - /** @description `date`, `-date` or `relevance` (best match first; needs a free-text word in `q`). Default `date`, oldest first. */ + /** @description `date`, `-date` or `relevance` (best match first; needs a free-text word in `q`). Default `relevance` when `q` has a free-text word, and `-date`, newest first, when it has none. */ sort?: string; }; header?: never; @@ -11926,7 +11981,7 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["Page_Message"]; + "application/json": components["schemas"]["ListMessagesResponse"]; }; }; /** @description [`authentication-required`](https://messagecrate.app/docs/developer/reference/errors/authentication-required): The request carried no usable credential: the `Authorization: Bearer ` header is missing, malformed, unknown or expired. */ diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index c278d1df0..d76e5808b 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -31,6 +31,15 @@ export type MessageTapback = Schema["Tapback"]; * same row shape the Export routes return, since one loader serves both. */ export type Message = Schema["Message"]; +/** + * A Messages search as the server read it: the order `GET /v1/messages` + * applied and the free-text terms it ranks by, which the Messages list bolds. + */ +export type MessageSearch = Schema["MessageSearch"]; + +/** One free-text term the server ranks a Messages search by: a word or a phrase. */ +export type FreeTextTerm = Schema["FreeTextTerm"]; + export type AttachmentMediaMode = "copy" | "convert" | "compress" | "skip"; export interface ExtractConfig { diff --git a/web/src/screens/MessageSearchList.test.tsx b/web/src/screens/MessageSearchList.test.tsx index 7ee1c8504..168ff954e 100644 --- a/web/src/screens/MessageSearchList.test.tsx +++ b/web/src/screens/MessageSearchList.test.tsx @@ -4,6 +4,8 @@ import { cleanup, render, screen, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { MessageSearchSort } from "../lib/messageSearchSort"; import { listMessages } from "../lib/serverApi"; +import type { MessageSearch } from "../lib/types"; +import { message } from "../test/apiShapes"; import { mockedAuth, Providers } from "../test/providers"; import { setupUser } from "../test/user"; import MessageSearchList from "./MessageSearchList"; @@ -17,6 +19,21 @@ vi.mock("../lib/serverApi", async (importOriginal) => ({ const listMessagesMock = vi.mocked(listMessages); +// jsdom lays nothing out, so the real list would measure no room for a row. +// Every row is drawn here, which is all these tests need of it. +vi.mock("../components/VirtualList", async () => { + const { createElement } = await import("react"); + return { + default: ({ + count, + renderItem, + }: { + count: number; + renderItem: (index: number) => import("react").ReactNode; + }) => createElement("div", null, ...Array.from({ length: count }, (_, i) => renderItem(i))), + }; +}); + // jsdom has no ResizeObserver; VirtualList observes its scroll container on mount. class StubResizeObserver { observe() {} @@ -27,9 +44,14 @@ class StubResizeObserver { beforeEach(() => { vi.stubGlobal("ResizeObserver", StubResizeObserver); listMessagesMock.mockReset(); - listMessagesMock.mockResolvedValue({ items: [], total: 12408, limit: 40, offset: 0 }); + answer({ sort: "relevance", terms: [{ text: "photo", prefix: false }] }); }); +/** The server answers every page with `search`, and 12,408 messages in all. */ +function answer(search: MessageSearch, items = [] as ReturnType[]) { + listMessagesMock.mockResolvedValue({ items, total: 12408, limit: 40, offset: 0, search }); +} + afterEach(() => { vi.unstubAllGlobals(); cleanup(); @@ -65,32 +87,52 @@ describe("MessageSearchList", () => { expect(listMessagesMock).not.toHaveBeenCalled(); }); - it("shows the total, and sorts by relevance by default for a free-text word", async () => { + it("sends no sort, and shows the order the server applied", async () => { renderList("from:Alice photo"); expect(await screen.findByText("12,408 messages")).toBeVisible(); expect(listMessagesMock).toHaveBeenCalledWith( - { q: "from:Alice photo", sort: "relevance", limit: 40, offset: 0 }, + { q: "from:Alice photo", limit: 40, offset: 0 }, expect.anything(), ); expect(screen.getByRole("button", { name: "Sort messages by Relevance" })).toBeVisible(); expect(await sortChoices()).toEqual(["Relevance", "Date"]); }); - it("offers only Date, newest first, when the search has no free-text word", async () => { + it("offers only Date when the server returns no terms to rank by", async () => { + answer({ sort: "-date", terms: [] }); renderList("from:Alice date:2024"); - await waitFor(() => - expect(listMessagesMock).toHaveBeenCalledWith( - expect.objectContaining({ q: "from:Alice date:2024", sort: "-date" }), - expect.anything(), - ), + expect( + await screen.findByRole("button", { name: "Sort messages by Date, Newest first" }), + ).toBeVisible(); + expect(listMessagesMock).toHaveBeenCalledWith( + { q: "from:Alice date:2024", limit: 40, offset: 0 }, + expect.anything(), ); + expect(await sortChoices()).toEqual(["Date", "Oldest first", "Newest first"]); + }); + + it("takes the order and the terms from the server, not from the words typed", async () => { + // A free-text word the server reports no term for: the menu follows the + // server, and offers no Relevance. + answer({ sort: "-date", terms: [] }); + renderList("photo"); expect( - screen.getByRole("button", { name: "Sort messages by Date, Newest first" }), + await screen.findByRole("button", { name: "Sort messages by Date, Newest first" }), ).toBeVisible(); expect(await sortChoices()).toEqual(["Date", "Oldest first", "Newest first"]); }); - it("keeps a picked date order, and hands a new pick to the caller", async () => { + it("bolds the terms the server returned", async () => { + answer({ sort: "relevance", terms: [{ text: "dent", prefix: true }] }, [ + message({ id: 7, text: "Here is the photo from the dentist" }), + ]); + renderList("whatever was typed"); + const row = await screen.findByRole("button", { name: /photo from the dentist/ }); + expect([...row.querySelectorAll("strong")].map((b) => b.textContent)).toEqual(["dentist"]); + }); + + it("sends a picked date order, and hands a new pick to the caller", async () => { + answer({ sort: "date", terms: [{ text: "photo", prefix: false }] }); const { onSortPick } = renderList("photo", { sort: "date", order: "asc" }); await waitFor(() => expect(listMessagesMock).toHaveBeenCalledWith( @@ -99,7 +141,9 @@ describe("MessageSearchList", () => { ), ); const user = setupUser(); - await user.click(screen.getByRole("button", { name: "Sort messages by Date, Oldest first" })); + await user.click( + await screen.findByRole("button", { name: "Sort messages by Date, Oldest first" }), + ); await user.click(screen.getByRole("menuitemradio", { name: "Relevance" })); expect(onSortPick).toHaveBeenCalledWith({ sort: "relevance", order: "asc" }); }); diff --git a/web/src/screens/MessageSearchList.tsx b/web/src/screens/MessageSearchList.tsx index bbbc300c2..33c89a540 100644 --- a/web/src/screens/MessageSearchList.tsx +++ b/web/src/screens/MessageSearchList.tsx @@ -1,22 +1,22 @@ -import { type ReactNode, useCallback, useMemo } from "react"; +import { useCallback } from "react"; import ListRangeHeader from "../components/ListRangeHeader"; import MessageSearchRow from "../components/MessageSearchRow"; import SortMenu, { type SortField } from "../components/SortMenu"; import VirtualList from "../components/VirtualList"; import { apiErrorMessage } from "../lib/apiErrorMessage"; -import { freeTextTerms } from "../lib/freeTextTerms"; import { MAX_LIST_OFFSET } from "../lib/listPaging"; import { messageCount } from "../lib/messageRowText"; import { - effectiveMessageSort, type MessageSearchSort, type MessageSearchSortKey, + type MessageSortParam, + messageSortFromParam, messageSortParam, } from "../lib/messageSearchSort"; import { keys } from "../lib/queryKeys"; import { type PagedFetchPage, useRoutePagedList } from "../lib/routeQuery"; import { listMessages } from "../lib/serverApi"; -import type { Message } from "../lib/types"; +import type { FreeTextTerm, Message, MessageSearch } from "../lib/types"; import { useDebouncedQuery } from "../lib/useDebouncedQuery"; /** Rows read at a time as the person scrolls. */ @@ -25,16 +25,24 @@ const PAGE_SIZE = 40; const RELEVANCE: SortField = { id: "relevance", label: "Relevance" }; const DATE: SortField = { id: "date", label: "Date" }; +/** No terms to bold: one shared value, so a row's memo sees the same array. */ +const NO_TERMS: readonly FreeTextTerm[] = []; + +/** What a page of `GET /v1/messages` says beside its rows: the server's reading of the search. */ +type MessagesPageExtra = { search: MessageSearch }; + /** * The Messages list: one row per message the search matches, from every * conversation (#313). It reads 40 rows at a time as the person scrolls and * shows the total, up to the route's offset ceiling of 50,000. An empty * search lists nothing and asks for one. * - * The sort menu offers Relevance only when the query has a free-text word - * to rank by, and it is then the default; otherwise the default is Date, - * newest first. A pick is the caller's to keep, so it survives opening a - * result. + * The server picks the order unless the person picked one, and every page + * says which order it applied and which free-text terms it ranks by (#1538). + * The sort menu shows that order, offers Relevance only when terms came + * back, and the rows bold those terms, so nothing here parses the search to + * guess what the server will do. A pick is the caller's to keep, so it + * survives opening a result. */ export default function MessageSearchList({ query, @@ -50,24 +58,7 @@ export default function MessageSearchList({ onSelect: (message: Message) => void; }) { const debouncedQ = useDebouncedQuery(query); - const q = debouncedQ.trim(); - const terms = useMemo(() => freeTextTerms(q), [q]); - const rankable = terms.length > 0; - const sort = effectiveMessageSort(sortPick, rankable); - - const sortMenu = ( - - ); if (!q) { return ( @@ -80,13 +71,14 @@ export default function MessageSearchList({ ); } + const sortParam = sortPick ? messageSortParam(sortPick) : null; return ( @@ -95,34 +87,54 @@ export default function MessageSearchList({ function MessageResults({ q, - sort, - terms, - sortMenu, + sortParam, + sortPick, + onSortPick, selectedId, onSelect, }: { q: string; - sort: MessageSearchSort; - terms: ReturnType; - sortMenu: ReactNode; + /** The `sort` to send, or null to let the server pick. */ + sortParam: MessageSortParam | null; + sortPick: MessageSearchSort | null; + onSortPick: (next: MessageSearchSort) => void; selectedId: number | null; onSelect: (message: Message) => void; }) { - const sortParam = messageSortParam(sort); - const fetchPage = useCallback>( + const fetchPage = useCallback>( async ({ limit, offset, signal }) => { - const res = await listMessages({ q, sort: sortParam, limit, offset }, { signal }); - return { items: res.items, total: res.total }; + const res = await listMessages( + sortParam ? { q, sort: sortParam, limit, offset } : { q, limit, offset }, + { signal }, + ); + return { items: res.items, total: res.total, search: res.search }; }, [q, sortParam], ); - const { items, total, loading, refreshing, filling, error, hasMore, loadMore } = - useRoutePagedList(keys.messages.list(q, sortParam), fetchPage, { + const { items, total, lastPage, loading, refreshing, filling, error, hasMore, loadMore } = + useRoutePagedList(keys.messages.list(q, sortParam ?? ""), fetchPage, { firstPageSize: PAGE_SIZE, fillPageSize: PAGE_SIZE, maxOffset: MAX_LIST_OFFSET, }); + const search = lastPage?.search ?? null; + const terms = search?.terms ?? NO_TERMS; + // Until the first page says which order it is in, the menu shows the + // person's pick, or nothing when the server is picking. + const sort = search ? messageSortFromParam(search.sort) : sortPick; + const sortMenu = sort ? ( + 0 ? [RELEVANCE, DATE] : [DATE]} + unordered={["relevance"]} + sort={sort.sort} + order={sort.order} + onChange={onSortPick} + itemNoun="messages" + ascLabel="Oldest first" + descLabel="Newest first" + /> + ) : null; if (error && items.length === 0) { return (
diff --git a/web/src/screens/message/useConversationMessages.test.tsx b/web/src/screens/message/useConversationMessages.test.tsx index 945d7df07..94aab4620 100644 --- a/web/src/screens/message/useConversationMessages.test.tsx +++ b/web/src/screens/message/useConversationMessages.test.tsx @@ -53,6 +53,9 @@ function deferred() { return { promise, resolve, reject }; } +/** What `GET /v1/messages` says of a Find search: newest first, as Find asks. */ +const FIND_SEARCH = { sort: "-date", terms: [] }; + type MessagePage = { items: Message[]; total: number; limit: number; offset: number }; function page(items: Message[]): MessagePage { return { items, total: items.length, limit: 50, offset: 0 }; @@ -215,6 +218,7 @@ describe("useConversationMessages", () => { })) as unknown as typeof listConversationMessages); // The newest match is found only by its earlier version; the older one by its text. searchMessages.mockResolvedValue({ + search: FIND_SEARCH, items: [ { ...message(60), @@ -259,6 +263,7 @@ describe("useConversationMessages", () => { }); // `pizz` finds message 60 only by its first version; `pizza tonight` by its second. searchMessages.mockImplementation((async ({ q }: { q?: string }) => ({ + search: FIND_SEARCH, items: [ { ...message(60), @@ -296,6 +301,7 @@ describe("useConversationMessages", () => { offset: 0, })) as unknown as typeof listConversationMessages); searchMessages.mockResolvedValue({ + search: FIND_SEARCH, items: [ { ...message(60), @@ -353,7 +359,13 @@ describe("useConversationMessages", () => { limit: 50, offset: params.around === undefined ? 98 : 0, })) as unknown as typeof listConversationMessages); - searchMessages.mockResolvedValue({ items: [message(3)], total: 40, limit: 1, offset: 0 }); + searchMessages.mockResolvedValue({ + search: FIND_SEARCH, + items: [message(3)], + total: 40, + limit: 1, + offset: 0, + }); const { result } = renderHook(() => useConversationMessages(7), { wrapper: Providers }); await waitFor(() => expect(result.current.loading).toBe(false)); @@ -381,6 +393,7 @@ describe("useConversationMessages", () => { offset: 0, })) as unknown as typeof listConversationMessages); searchMessages.mockResolvedValue({ + search: FIND_SEARCH, items: [message(60), message(20)], total: 2, limit: 50, @@ -430,6 +443,7 @@ describe("useConversationMessages", () => { page([message(params.around ?? matches)])) as unknown as typeof listConversationMessages); // The server pages the matches 50 at a time, newest first, and counts all of them. searchMessages.mockImplementation(async ({ offset = 0, limit = 50 }) => ({ + search: FIND_SEARCH, items: Array.from({ length: Math.max(0, Math.min(limit, matches - offset)) }, (_, i) => message(matches - offset - i), ),