diff --git a/server/src/crate_server/handlers/browse.gleam b/server/src/crate_server/handlers/browse.gleam index 765fec3..1f3dea0 100644 --- a/server/src/crate_server/handlers/browse.gleam +++ b/server/src/crate_server/handlers/browse.gleam @@ -60,14 +60,18 @@ pub fn browse(req: Request, ctx: Context) -> Response { list.key_find(query, "limit") |> result.try(int.parse) |> option.from_result - let #(page, next_cursor) = - pagination.page( - rows, - fn(resolved) { resolved.row.uri }, - cursor_namespace(q, genre, chosen), - cursor, - Some(option.unwrap(limit, pagination.default_limit)), - ) + let namespace = cursor_namespace(q, genre, chosen) + let #(page, next_cursor) = case anchor_lost(rows, namespace, cursor) { + True -> #([], None) + False -> + pagination.page( + rows, + fn(resolved) { resolved.row.uri }, + namespace, + cursor, + Some(option.unwrap(limit, pagination.default_limit)), + ) + } json.object( list.flatten([ [#("releases", json.array(page, encode_row(own, _)))], @@ -78,6 +82,21 @@ pub fn browse(req: Request, ctx: Context) -> Response { |> wisp.json_response(200) } +// The anchor row lost its variant contest since the cursor was minted, so +// `pagination.page` would restart from the top: a page the caller already +// holds, under a cursor that can lead back to it. Only a decodable token +// counts as lost; garbage still restarts. +fn anchor_lost( + rows: List(browse_domain.ResolvedRelease), + namespace: String, + cursor: option.Option(String), +) -> Bool { + case option.then(cursor, pagination.decode_cursor(namespace, _)) { + None -> False + Some(uri) -> !list.any(rows, fn(resolved) { resolved.row.uri == uri }) + } +} + fn cursor_namespace( q: Result(String, Nil), genre: Result(String, Nil), diff --git a/server/test/browse_handler_test.gleam b/server/test/browse_handler_test.gleam index d94ded8..d591340 100644 --- a/server/test/browse_handler_test.gleam +++ b/server/test/browse_handler_test.gleam @@ -1,7 +1,7 @@ //// End-to-end test for the browse handler's `?strategy=` param: HTTP wiring //// only, domain correctness lives in catalog_variants/stats/strategy_test. -import crate_server/catalog/row.{BrowseRow} +import crate_server/catalog/row.{type BrowseRow, BrowseRow} import crate_server/catalog/source.{type Source, Source} import crate_server/context.{type Context} import crate_server/handlers/browse.{browse} @@ -132,50 +132,56 @@ pub fn resolved_release_includes_format_label_and_master_test() { ) } -fn release_source(count: Int) -> Source { - let rows = - int.range(1, count + 1, [], fn(rows, n) { - let id = int.to_string(n) - let row = - BrowseRow( - uri: "at://did:plc:pub/dev.mokkenstorm.crate.catalog.release/" <> id, - cid: "bafy" <> id, - title: "Release " <> id, - artist_display: None, - genres: [], - styles: [], - released: None, - country: None, - cover: None, - thumb_url: None, - discogs_id: None, - created_at: id, - publisher_did: "did:plc:pub", - publisher_handle: "pub.test", - publisher_pds: "https://pds.test", - supersedes: None, - based_on: None, - format: None, - label: None, - master: None, - barcode: None, - ) - [row, ..rows] - }) +fn source_of(rows: List(BrowseRow)) -> Source { Source(releases: fn() { rows }, adoption_count: fn(_uri) { 0 }) } -fn paginated_context(count: Int) -> #(Context, config.Config) { +fn release_rows(count: Int) -> List(BrowseRow) { + int.range(1, count + 1, [], fn(rows, n) { + let id = int.to_string(n) + let row = + BrowseRow( + uri: "at://did:plc:pub/dev.mokkenstorm.crate.catalog.release/" <> id, + cid: "bafy" <> id, + title: "Release " <> id, + artist_display: None, + genres: [], + styles: [], + released: None, + country: None, + cover: None, + thumb_url: None, + discogs_id: None, + created_at: id, + publisher_did: "did:plc:pub", + publisher_handle: "pub.test", + publisher_pds: "https://pds.test", + supersedes: None, + based_on: None, + format: None, + label: None, + master: None, + barcode: None, + ) + [row, ..rows] + }) +} + +fn context_for(rows: List(BrowseRow)) -> #(Context, config.Config) { let cfg = support.stub_config_with( support.unreachable_client(), "https://resolver.test", "http://localhost:8080", ) - let ctx = support.stub_context_with_variant_source(cfg, release_source(count)) + let ctx = support.stub_context_with_variant_source(cfg, source_of(rows)) #(ctx, cfg) } +fn paginated_context(count: Int) -> #(Context, config.Config) { + context_for(release_rows(count)) +} + fn release_uris(body: String) -> List(String) { let decoder = decode.at(["releases"], decode.list(decode.at(["uri"], decode.string))) @@ -200,6 +206,23 @@ pub fn browse_pages_through_the_full_catalog_with_an_opaque_cursor_test() { }) } +/// A cursor whose anchor lost its variant contest between requests must not +/// hand back a page the caller already has, under a cursor leading to it +/// again: that is a LOAD MORE that stays lit and never advances. +pub fn browse_reports_no_progress_when_the_cursor_anchor_is_gone_test() { + let rows = release_rows(4) + let #(ctx, cfg) = context_for(rows) + let first = browse_body("?limit=2", ctx, cfg) + let assert Ok(anchor) = list.last(release_uris(first)) + let #(shifted, shifted_cfg) = + context_for(list.filter(rows, fn(row) { row.uri != anchor })) + let second = + browse_body("?limit=2&cursor=" <> next_cursor(first), shifted, shifted_cfg) + + assert release_uris(second) == [] + assert !string.contains(second, "\"cursor\"") +} + pub fn browse_omits_cursor_after_the_last_page_test() { let #(ctx, cfg) = paginated_context(1) let body = browse_body("?limit=2", ctx, cfg) diff --git a/web/src/crate_web.gleam b/web/src/crate_web.gleam index c62be99..886a6a2 100644 --- a/web/src/crate_web.gleam +++ b/web/src/crate_web.gleam @@ -57,6 +57,7 @@ fn init(_flags) -> #(Model, Effect(Msg)) { amend: model.blank_amend(), avatar: None, browse: [], + browse_loading: active_route == model.Browse, browse_cursor: None, browse_loading_more: False, browse_adding: None, diff --git a/web/src/crate_web/model.gleam b/web/src/crate_web/model.gleam index 0ee7848..0bb959c 100644 --- a/web/src/crate_web/model.gleam +++ b/web/src/crate_web/model.gleam @@ -796,6 +796,9 @@ pub type Model { amend: AmendDraft, avatar: Option(String), browse: List(BrowseRelease), + // True while the grid's own (re)load is in flight; page-scoped, so a + // browse response can never leave another page's controls disabled. + browse_loading: Bool, // The current filtered catalog's next-page cursor; `None` means there is // no next page or a changed filter has invalidated the prior cursor. browse_cursor: Option(String), diff --git a/web/src/crate_web/pages/browse.gleam b/web/src/crate_web/pages/browse.gleam index c9fba93..851693d 100644 --- a/web/src/crate_web/pages/browse.gleam +++ b/web/src/crate_web/pages/browse.gleam @@ -28,7 +28,7 @@ pub fn view(model: Model) -> Element(Msg) { html.div([attr.class("body list-body")], [ search_bar(model), html.div([attr.class("list-scroll")], [ - grid(model.browse, model.browse_adding, model.busy), + grid(model.browse, model.browse_adding, model.browse_loading), load_more(model), ]), ]), @@ -162,9 +162,9 @@ pub fn genre_suggestions( fn grid( rows: List(BrowseRelease), adding: Option(#(String, String)), - busy: Bool, + loading: Bool, ) -> Element(Msg) { - case rows, busy { + case rows, loading { [], False -> html.p([attr.class("hint")], [ text("Nothing in the shared catalog yet."), diff --git a/web/src/crate_web/update/browse.gleam b/web/src/crate_web/update/browse.gleam index 5867fdb..1f9114f 100644 --- a/web/src/crate_web/update/browse.gleam +++ b/web/src/crate_web/update/browse.gleam @@ -15,6 +15,17 @@ import gleam/option.{type Option, None, Some} import gleam/string import lustre/effect.{type Effect} +/// The one place a catalog request is paired with the filters it is tagged +/// with. `got_browse`/`got_browse_search` discard any response whose tag +/// disagrees with the model, so a request chosen anywhere else risks being +/// tagged for filters the model doesn't hold, and then never landing. +pub fn reload(model: Model) -> Effect(Msg) { + case string.trim(model.browse_query), string.trim(model.browse_genre) { + "", "" -> load_browse() + _, _ -> search_browse(model.browse_query, model.browse_genre) + } +} + pub fn got_browse( model: Model, query: String, @@ -34,7 +45,7 @@ pub fn got_browse( browse: data.releases, browse_cursor: data.cursor, browse_loading_more: False, - busy: False, + browse_loading: False, ), effect.none(), ) @@ -43,6 +54,7 @@ pub fn got_browse( Model( ..model.set_crate(model, OwnCrate, ShelfLoaded(Own, [])), auth: LoggedOut, + browse_loading: False, busy: False, ), effect.none(), @@ -50,13 +62,17 @@ pub fn got_browse( True, Error(xrpc.BadStatus(..)) -> #( Model( ..model, - busy: False, + browse_loading: False, notice: failed("Couldn't load the shared catalog (server error)."), ), effect.none(), ) True, Error(_) -> #( - Model(..model, busy: False, notice: failed("Couldn't reach the server.")), + Model( + ..model, + browse_loading: False, + notice: failed("Couldn't reach the server."), + ), effect.none(), ) } @@ -74,7 +90,7 @@ pub fn browse_query(model: Model, value: String) -> #(Model, Effect(Msg)) { browse_loading_more: False, ) case string.trim(value), string.trim(updated.browse_genre) { - "", "" -> #(updated, load_browse()) + "", "" -> #(updated, reload(updated)) _, _ -> #( updated, effects.debounce("browse-search", 300, msg.TriggerBrowseSearch), @@ -91,7 +107,7 @@ pub fn browse_genre(model: Model, value: String) -> #(Model, Effect(Msg)) { browse_loading_more: False, ) case string.trim(updated.browse_query), string.trim(value) { - "", "" -> #(updated, load_browse()) + "", "" -> #(updated, reload(updated)) _, _ -> #( updated, effects.debounce("browse-search", 300, msg.TriggerBrowseSearch), @@ -100,7 +116,7 @@ pub fn browse_genre(model: Model, value: String) -> #(Model, Effect(Msg)) { } pub fn trigger_browse_search(model: Model) -> #(Model, Effect(Msg)) { - #(model, search_browse(model.browse_query, model.browse_genre)) + #(model, reload(model)) } pub fn got_browse_search( @@ -122,13 +138,18 @@ pub fn got_browse_search( browse: data.releases, browse_cursor: data.cursor, browse_loading_more: False, + browse_loading: False, ), effect.none(), ) // A transient search failure keeps whatever's already on screen instead // of wiping the grid out from under the viewer mid-search. True, Error(_) -> #( - Model(..model, notice: failed("Couldn't search the shared catalog.")), + Model( + ..model, + browse_loading: False, + notice: failed("Couldn't search the shared catalog."), + ), effect.none(), ) } @@ -142,16 +163,15 @@ pub fn toggle_browse_filters(model: Model) -> #(Model, Effect(Msg)) { } pub fn browse_clear_filters(model: Model) -> #(Model, Effect(Msg)) { - #( + let updated = Model( ..model, browse_query: "", browse_genre: "", browse_cursor: None, browse_loading_more: False, - ), - load_browse(), - ) + ) + #(updated, reload(updated)) } pub fn browse_show_more(model: Model) -> #(Model, Effect(Msg)) { diff --git a/web/src/crate_web/update/routing.gleam b/web/src/crate_web/update/routing.gleam index c077c45..055c949 100644 --- a/web/src/crate_web/update/routing.gleam +++ b/web/src/crate_web/update/routing.gleam @@ -1,6 +1,4 @@ -import crate_web/effects.{ - discogs_status, load_browse, load_entry, load_feed, load_shelf, -} +import crate_web/effects.{discogs_status, load_entry, load_feed, load_shelf} import crate_web/model.{ type Model, Add, Browse, ConnectionsFollowers, ConnectionsFollowing, ConnectionsLoading, EditInbox, EditProposalDetail, Feed, FeedLoading, Model, @@ -146,8 +144,12 @@ pub fn on_route_change( Browse -> False _ -> model.browse_loading_more }, + browse_loading: case route { + Browse -> True + _ -> model.browse_loading + }, busy: case route { - Browse | EditInbox | EditProposalDetail(_) -> True + EditInbox | EditProposalDetail(_) -> True _ -> model.busy }, scan: scan_state, @@ -210,7 +212,7 @@ pub fn on_route_change( scan.start_capture(scan_state.mode), effects.scan_seen(), ]) - Browse -> load_browse() + Browse -> browse.reload(updated) Feed -> load_feed(model.FeedRequest(Feed, updated.feed_generation, None)) ConnectionsFollowing | ConnectionsFollowers -> diff --git a/web/test/browse_test.gleam b/web/test/browse_test.gleam index eb7d38c..2223591 100644 --- a/web/test/browse_test.gleam +++ b/web/test/browse_test.gleam @@ -2,7 +2,8 @@ import crate/gen/catalog/list_releases.{ReleaseRow} import crate_web/model.{type BrowseRelease, Model} import crate_web/msg.{ BrowseAdd, BrowseClearFilters, BrowseData, BrowseShowMore, GotBrowse, - GotBrowseAdd, GotBrowseMore, GotBrowseSearch, ToggleBrowseFilters, + GotBrowseAdd, GotBrowseMore, GotBrowseSearch, OnRouteChange, + ToggleBrowseFilters, } import crate_web/pages/browse import crate_web/update.{update} @@ -49,9 +50,36 @@ pub fn got_browse_stores_rows_test() { update(logged_in(), GotBrowse("", "", Ok(BrowseData(rows, Some("next"))))) assert model.browse == rows assert model.browse_cursor == Some("next") + assert model.browse_loading == False +} + +pub fn entering_browse_leaves_the_rest_of_the_app_alone_test() { + let seeded = Model(..logged_in(), browse_query: "slint") + let #(model, _) = update(seeded, OnRouteChange(model.Browse)) + assert model.browse_loading == True assert model.busy == False } +/// The reload on entry has to be tagged with the filters the model still +/// holds, or the staleness guard drops its own response and the grid never +/// leaves its loading state. +pub fn re_entering_browse_reloads_the_search_that_is_still_typed_test() { + let rows = [a_browse_release("at://uri/1")] + let seeded = Model(..logged_in(), browse_query: "slint") + let #(entered, _) = update(seeded, OnRouteChange(model.Browse)) + let #(landed, _) = + update( + entered, + GotBrowseSearch("slint", "", Ok(BrowseData(rows, Some("next")))), + ) + + assert entered.browse_query == "slint" + assert landed.browse == rows + assert landed.browse_cursor == Some("next") + assert landed.browse_loading == False + assert string.contains(browse.view(landed) |> element.to_string, "LOAD MORE") +} + pub fn stale_browse_load_is_discarded_once_a_filter_is_typed_test() { let filtered = a_browse_release("at://uri/1") let unfiltered = a_browse_release("at://uri/stale") @@ -177,6 +205,16 @@ pub fn browse_view_renders_a_reachable_load_more_button_test() { |> browse.view |> element.to_string assert string.contains(html, "LOAD MORE") + assert !string.contains(html, "disabled") +} + +pub fn browse_view_disables_load_more_while_the_next_page_is_in_flight_test() { + let html = + Model(..logged_in(), browse_cursor: Some("next"), browse_loading_more: True) + |> browse.view + |> element.to_string + assert string.contains(html, "LOADING…") + assert string.contains(html, "disabled") } pub fn browse_add_marks_in_flight_test() { diff --git a/web/test/record_test.gleam b/web/test/record_test.gleam index 7f25573..52d0166 100644 --- a/web/test/record_test.gleam +++ b/web/test/record_test.gleam @@ -11,6 +11,7 @@ import crate_web/msg.{ import crate_web/pages/record import crate_web/update.{update} import gleam/dict +import gleam/list import gleam/option.{type Option, None, Some} import gleam/string import lustre/element @@ -51,12 +52,20 @@ pub fn record_view_omits_the_sleeve_grade_card_when_absent_test() { assert !string.contains(html, "SLEEVE") } -pub fn record_view_hides_paid_and_from_rows_when_present_test() { - let html = record.view(logged_in(), a_detail()) |> element.to_string - assert !string.contains(html, "PAID") - assert !string.contains(html, "EUR 40.00") - assert !string.contains(html, "FROM") - assert !string.contains(html, "Record shop") +/// Both cases in one block: the rows are gone from the view, so a stored +/// price/counterparty renders exactly like an absent one. +pub fn record_view_never_shows_paid_or_from_rows_test() { + [ + an_entry(), + list_entries.Entry(..an_entry(), price: None, counterparty: None), + ] + |> list.each(fn(entry) { + let html = record.view(logged_in(), detail(entry)) |> element.to_string + assert !string.contains(html, "PAID") + assert !string.contains(html, "EUR 40.00") + assert !string.contains(html, "FROM") + assert !string.contains(html, "Record shop") + }) } pub fn public_detail_rows_hide_paid_and_from_when_present_test() { @@ -70,17 +79,6 @@ pub fn public_detail_rows_hide_paid_and_from_when_present_test() { assert !string.contains(html, "Record shop") } -pub fn record_view_omits_paid_and_from_rows_when_absent_test() { - let html = - record.view( - logged_in(), - detail(list_entries.Entry(..an_entry(), price: None, counterparty: None)), - ) - |> element.to_string - assert !string.contains(html, "PAID") - assert !string.contains(html, "FROM") -} - pub fn record_view_shows_the_provenance_line_when_source_is_present_test() { let source = src(Some("scan"), Some("discogs")) let html = diff --git a/web/test/support.gleam b/web/test/support.gleam index 6accb64..459ab6d 100644 --- a/web/test/support.gleam +++ b/web/test/support.gleam @@ -66,6 +66,7 @@ pub fn base() -> Model { amend: blank_amend(), avatar: None, browse: [], + browse_loading: False, browse_cursor: None, browse_loading_more: False, browse_adding: None,