From 0af89668681699e1330d23ecae76e927e47daf5f Mon Sep 17 00:00:00 2001 From: Sebastian Jeltsch Date: Tue, 3 Feb 2026 16:02:47 +0100 Subject: [PATCH] Cursors fixes: only include cursors where supported and fix admin UI's cursor state handling. --- .../assets/js/admin/src/components/Table.tsx | 6 +- .../js/admin/src/components/logs/LogsPage.tsx | 140 ++++++++++-------- .../admin/src/components/tables/TablePane.tsx | 116 +++++++-------- crates/assets/js/admin/src/lib/api/logs.ts | 2 + crates/assets/js/admin/src/lib/list.ts | 6 +- crates/core/src/admin/list_logs.rs | 26 +++- crates/core/src/admin/rows/list_rows.rs | 26 ++-- crates/core/src/records/list_records.rs | 59 ++++---- crates/core/src/records/subscribe.rs | 3 +- 9 files changed, 212 insertions(+), 172 deletions(-) diff --git a/crates/assets/js/admin/src/components/Table.tsx b/crates/assets/js/admin/src/components/Table.tsx index bae605fc..38c42da7 100644 --- a/crates/assets/js/admin/src/components/Table.tsx +++ b/crates/assets/js/admin/src/components/Table.tsx @@ -39,6 +39,8 @@ import { Checkbox } from "@/components/ui/checkbox"; import { Skeleton } from "@/components/ui/skeleton"; import { createIsMobile } from "@/lib/signals"; +export type { Updater } from "@tanstack/solid-table"; + type TableOptions = { data: TData[] | undefined; columns: ColumnDef[]; @@ -57,7 +59,9 @@ export function buildTable( opts: TableOptions, overrides?: Partial>, ) { - console.debug("buildTable: ", opts); + console.debug( + `buildTable(): columns=${opts.columns.length}, rows=${opts.data?.length}`, + ); function buildColumns() { const onRowSelection = opts.onRowSelection; diff --git a/crates/assets/js/admin/src/components/logs/LogsPage.tsx b/crates/assets/js/admin/src/components/logs/LogsPage.tsx index 79002a3e..0687b1e5 100644 --- a/crates/assets/js/admin/src/components/logs/LogsPage.tsx +++ b/crates/assets/js/admin/src/components/logs/LogsPage.tsx @@ -10,13 +10,12 @@ import { createMemo, } from "solid-js"; import { useSearchParams } from "@solidjs/router"; -import { createWritableMemo } from "@solid-primitives/memo"; import type { ColumnDef, PaginationState, SortingState, } from "@tanstack/solid-table"; -import { useQuery, useQueryClient } from "@tanstack/solid-query"; +import { useQuery } from "@tanstack/solid-query"; import { Chart } from "chart.js/auto"; import type { ChartData, @@ -44,6 +43,7 @@ import { DialogFooter, } from "@/components/ui/dialog"; import { Table, buildTable } from "@/components/Table"; +import type { Updater } from "@/components/Table"; import { FilterBar } from "@/components/FilterBar"; import { fetchLogs } from "@/lib/api/logs"; @@ -164,74 +164,98 @@ const columns: ColumnDef[] = [ }, ]; +type SearchParams = { + filter?: string; + pageSize?: string; + pageIndex?: string; +}; + // Value is the previous value in case this isn't the first fetch. export function LogsPage() { - const [searchParams, setSearchParams] = useSearchParams<{ - filter?: string; - pageSize?: string; - pageIndex?: string; - }>(); - const [cursors, setCursors] = createWritableMemo(() => { - // Reset when search params change - const _ = [searchParams.pageSize, searchParams.filter]; - console.debug("cursor reset"); - return []; + // IMPORTANT: We need to memo the search params to treat absence and defaults + // consistently, otherwise `undefined`->`default` may invalidate the cursors. + const [searchParams, setSearchParams] = useSearchParams(); + const filter = createMemo(() => searchParams.filter); + const pageSize = createMemo(() => safeParseInt(searchParams.pageSize) ?? 20); + const pageIndex = createMemo(() => safeParseInt(searchParams.pageIndex) ?? 0); + + const pagination = (): PaginationState => ({ + pageIndex: pageIndex(), + pageSize: pageSize(), }); - - const pagination = (): PaginationState => { - return { - pageSize: safeParseInt(searchParams.pageSize) ?? 20, - pageIndex: safeParseInt(searchParams.pageIndex) ?? 0, - }; - }; - - const setFilter = (filter: string | undefined) => { + const setPagination = (s: PaginationState) => { setSearchParams({ ...searchParams, + pageIndex: s.pageIndex, + pageSize: s.pageSize, + }); + }; + const setFilter = (filter: string | undefined) => { + // Reset pagination. + setSearchParams({ + pageIndex: undefined, + pageSize: undefined, filter, - // Reset - pageIndex: "0", }); }; - const [sorting, setSorting] = createSignal([]); + const [sorting, setSortingImpl] = createSignal([]); + const setSorting = (s: Updater) => { + // Reset pagination. + setSearchParams({ + ...searchParams, + pageIndex: undefined, + pageSize: undefined, + }); + setSortingImpl(s); + }; + + const cursors = createMemo>(() => { + // Reset cursor whenever table or search params change. This is basically + // the same as `queryKey` below minus `pageIndex`. + const _ = [pageSize(), filter(), sorting()]; + console.debug("resetting cursor"); + return new Map(); + }); // NOTE: admin user endpoint doesn't support offset, we have to cursor through // and cannot just jump to page N. const logsFetch = useQuery(() => ({ - queryKey: [ - "logs", - searchParams.filter, - pagination().pageSize, - pagination().pageIndex, - sorting(), - ], - queryFn: async () => { - const p = pagination(); - const c = cursors(); - const s = sorting(); + queryKey: [pagination(), filter(), sorting()], + queryFn: async ({ queryKey }) => { + console.debug(`Fetching data with key: ${queryKey}`); - const response = await fetchLogs( - p.pageSize, - searchParams.filter, - c[p.pageIndex - 1], - formatSortingAsOrder(s), - ); + try { + const { pageSize, pageIndex } = pagination(); + const cursor = cursors().get(pageIndex - 1); - const cursor = response.cursor; - if (cursor && p.pageIndex >= c.length) { - setCursors([...c, cursor]); + const response = await fetchLogs( + pageSize, + pageIndex, + filter(), + cursor, + formatSortingAsOrder(sorting()), + ); + + // Update cursors. + if (sorting().length === 0 && response.cursor) { + cursors().set(pageIndex, response.cursor); + } + + return response; + } catch (err) { + // Reset. + setSearchParams({ + filter: undefined, + pageSize: undefined, + pageIndex: undefined, + }); + + throw err; } - - return response; }, })); - const client = useQueryClient(); - const refetch = () => { - client.invalidateQueries({ - queryKey: ["logs"], - }); - }; + const refetch = () => logsFetch.refetch(); const [showMap, setShowMap] = createSignal(true); const [showGeoipDialog, setShowGeoipDialog] = createSignal(false); @@ -246,13 +270,7 @@ export function LogsPage() { onColumnPinningChange: setColumnPinningState, rowCount: Number(logsFetch.data?.total_row_count ?? -1), pagination: pagination(), - onPaginationChange: (s: PaginationState) => { - setSearchParams({ - ...searchParams, - pageIndex: s.pageIndex, - pageSize: s.pageSize, - }); - }, + onPaginationChange: setPagination, }, { manualSorting: true, @@ -338,9 +356,9 @@ export function LogsPage() { { - if (value === searchParams.filter) { + if (value === filter()) { refetch(); } else { setFilter(value); diff --git a/crates/assets/js/admin/src/components/tables/TablePane.tsx b/crates/assets/js/admin/src/components/tables/TablePane.tsx index 91d8c8f1..c57fcdb9 100644 --- a/crates/assets/js/admin/src/components/tables/TablePane.tsx +++ b/crates/assets/js/admin/src/components/tables/TablePane.tsx @@ -1,6 +1,5 @@ import { Match, Show, Switch, createMemo, createSignal, JSX } from "solid-js"; import type { Signal } from "solid-js"; -import { createWritableMemo } from "@solid-primitives/memo"; import { TbRefresh, TbTable, TbTrash, TbColumns } from "solid-icons/tb"; import { useSearchParams } from "@solidjs/router"; import { useQuery } from "@tanstack/solid-query"; @@ -43,6 +42,7 @@ import { DebugDialogButton } from "@/components/tables/SchemaDownload"; import { CreateAlterTableForm } from "@/components/tables/CreateAlterTable"; import { CreateAlterIndexForm } from "@/components/tables/CreateAlterIndex"; import { Table as TableComponent, buildTable } from "@/components/Table"; +import type { Updater } from "@/components/Table"; import { FilterBar } from "@/components/FilterBar"; import { DestructiveActionButton } from "@/components/DestructiveActionButton"; import { IconButton } from "@/components/IconButton"; @@ -935,6 +935,12 @@ function TriggerTable(props: { table: Table; schemas: ListSchemasResponse }) { ); } +type SearchParams = { + filter?: string; + pageSize?: string; + pageIndex?: string; +}; + export function TablePane(props: { selectedTable: [Table, string] | [View, string]; schemas: ListSchemasResponse; @@ -943,52 +949,57 @@ export function TablePane(props: { const selectedSchema = () => props.selectedTable[0]; const isTable = () => tableType(selectedSchema()) === "table"; - const [searchParams, setSearchParams] = useSearchParams<{ - filter?: string; - pageSize?: string; - pageIndex?: string; - }>(); + // IMPORTANT: We need to memo the search params to treat absence and defaults + // consistently, otherwise `undefined`->`default` may invalidate the cursors. + const [searchParams, setSearchParams] = useSearchParams(); + const filter = createMemo(() => searchParams.filter); + const pageSize = createMemo(() => safeParseInt(searchParams.pageSize) ?? 20); + const pageIndex = createMemo(() => safeParseInt(searchParams.pageIndex) ?? 0); - const [cursors, setCursors] = createWritableMemo(() => { - // Reset cursor whenever table or search params change. - const _ = [props.selectedTable, searchParams.pageSize, searchParams.filter]; - console.debug("resetting cursor"); - return []; + const pagination = (): PaginationState => ({ + pageIndex: pageIndex(), + pageSize: pageSize(), }); + const setPagination = (s: PaginationState) => { + setSearchParams({ + ...searchParams, + pageIndex: s.pageIndex, + pageSize: s.pageSize, + }); + }; + const setFilter = (filter: string | undefined) => { + // Reset pagination. + setSearchParams({ + pageIndex: undefined, + pageSize: undefined, + filter, + }); + }; - const [filter, setFilter] = [ - () => searchParams.filter, - (filter: string | undefined) => { - setSearchParams({ - ...searchParams, - filter, - }); - }, - ]; + const [sorting, setSortingImpl] = createSignal([]); + const setSorting = (s: Updater) => { + // Reset pagination. + setSearchParams({ + ...searchParams, + pageIndex: undefined, + pageSize: undefined, + }); + setSortingImpl(s); + }; - const [pagination, setPagination] = [ - (): PaginationState => { - return { - pageSize: safeParseInt(searchParams.pageSize) ?? 20, - pageIndex: safeParseInt(searchParams.pageIndex) ?? 0, - }; - }, - (s: PaginationState) => { - setSearchParams({ - ...searchParams, - pageSize: s.pageSize, - pageIndex: s.pageIndex, - }); - }, - ]; - - const [sorting, setSorting] = createSignal([]); + const cursors = createMemo>(() => { + // Reset cursor whenever table or search params change. This is basically + // the same as `queryKey` below minus `pageIndex`. + const _ = [selectedSchema(), pageSize(), filter(), sorting()]; + console.debug("resetting cursor"); + return new Map(); + }); const records: QueryObserverResult = useQuery(() => ({ queryKey: [ - selectedSchema().name, - searchParams.filter, + selectedSchema(), pagination(), + filter(), sorting(), ] as ReadonlyArray, queryFn: async ({ queryKey }) => { @@ -996,19 +1007,20 @@ export function TablePane(props: { try { const { pageSize, pageIndex } = pagination(); + const cursor = cursors().get(pageIndex - 1); const response = await fetchRows( selectedSchema().name, - searchParams.filter ?? null, + filter() ?? null, pageSize, pageIndex, - cursors()[pageIndex - 1], + cursor ?? null, formatSortingAsOrder(sorting()), ); - const newCursor = response.cursor; - if (newCursor && pageIndex >= cursors().length) { - setCursors([...cursors(), newCursor]); + // Update cursors. + if (sorting().length === 0 && response.cursor) { + cursors().set(pageIndex, response.cursor); } return response; @@ -1054,22 +1066,10 @@ export function TablePane(props: { - + - - - - log.id); } return log.id.to_string(); - }), + }) + } else { + None + }; + + let response = ListLogsResponse { + total_row_count, + cursor: next_cursor, entries: logs .into_iter() .map(|log| log.into()) @@ -258,6 +267,7 @@ async fn fetch_logs( geoip_db_type: Option, filter_where_clause: WhereClause, cursor: Option, + offset: Option, order: &Order, limit: usize, ) -> Result, Error> { @@ -268,6 +278,11 @@ async fn fetch_logs( trailbase_sqlite::Value::Integer(limit as i64), )); + params.push(( + Cow::Borrowed(":offset"), + trailbase_sqlite::Value::Integer(offset.map_or(0, |o| o.try_into().unwrap_or(0))), + )); + if let Some(cursor) = cursor { params.push(( Cow::Borrowed(":cursor"), @@ -301,6 +316,7 @@ async fn fetch_logs( ORDER BY {order_clause} LIMIT :limit + OFFSET :offset "#, geoip = match geoip_db_type { Some(DatabaseType::GeoLite2Country) => "geoip_country(log.client_ip) AS client_geoip_cc", diff --git a/crates/core/src/admin/rows/list_rows.rs b/crates/core/src/admin/rows/list_rows.rs index 8353b217..b8128782 100644 --- a/crates/core/src/admin/rows/list_rows.rs +++ b/crates/core/src/admin/rows/list_rows.rs @@ -107,18 +107,22 @@ pub async fn list_rows_handler( ) .await?; - let next_cursor = cursor_column.and_then(|(col_idx, _col)| { - let row = rows.last()?; - assert!(row.len() > col_idx); - match &row[col_idx] { - SqlValue::Integer(n) => Some(n.to_string()), - SqlValue::Blob(b) => { - // Should be a base64 encoded [u8; 16] id. - b.to_b64_url_safe().ok() + let next_cursor = if order.is_none() { + cursor_column.and_then(|(col_idx, _col)| { + let row = rows.last()?; + assert!(row.len() > col_idx); + match &row[col_idx] { + SqlValue::Integer(n) => Some(n.to_string()), + SqlValue::Blob(b) => { + // Should be a base64 encoded [u8; 16] id. + b.to_b64_url_safe().ok() + } + _ => None, } - _ => None, - } - }); + }) + } else { + None + }; return Ok(Json(ListRowsResponse { total_row_count, diff --git a/crates/core/src/records/list_records.rs b/crates/core/src/records/list_records.rs index 4636ea4e..f90441ce 100644 --- a/crates/core/src/records/list_records.rs +++ b/crates/core/src/records/list_records.rs @@ -11,7 +11,6 @@ use std::convert::TryInto; use std::sync::LazyLock; use trailbase_qs::{OrderPrecedent, Query}; use trailbase_schema::QualifiedNameEscaped; -use trailbase_schema::sqlite::ColumnDataType; use trailbase_sqlite::Value; use crate::app_state::AppState; @@ -127,13 +126,19 @@ pub async fn list_records_handler( )); } + // NOTE: We lost the ability to cursor VIEWs when we moved to `_rowid_`. They currently only + // support OFFSET. We could restore functionality where a cursor-able PK is included. + // NOTE: We cannot use cursors if there's a custom order/sorting defined. + // NOTE: Multiple order criteria only matter for non-unique columns, i.e. ordering on PK + // and then on another column makes no difference. + let supports_cursor = is_table + && order + .as_ref() + .is_none_or(|o| o.columns.is_empty() || (pk_column.name == o.columns[0].0)); let cursor_clause = if let Some(encrypted_cursor) = cursor { - if !is_table { - // TODO: When we moved to _rowid_ for cursoring, we lost the ability to cursor VIEWs. They - // currently only support OFFSET. We could restore cursoring for cases where a cursorable - // PK column is included. We also need a test-case to cover this. + if !supports_cursor { return Err(RecordError::BadRequest( - "Only TABLEs support cursors. Use offset for VIEWs.", + "Only TABLEs with PK primary order support cursors. Use offset instead.", )); } @@ -146,37 +151,29 @@ pub async fn list_records_handler( )?), )); - let mut pk_order = OrderPrecedent::Descending; - if let Some(ref order) = order - && let Some((col, ord)) = order.columns.first() - && *ord == OrderPrecedent::Ascending - { - if pk_column.data_type != ColumnDataType::Integer || *col != pk_column.name { - // NOTE: This relies on the fact that _rowid_ is an alias for integer primary key - // columns. - return Err(RecordError::BadRequest( - "Cannot cursor on queries where the primary order criterion is not an integer primary key", - )); + // We already check above that primary order criteria must be none or PK. + match order.as_ref() { + Some(ord) + if ord + .columns + .first() + .is_some_and(|(_c, o)| *o == OrderPrecedent::Ascending) => + { + Some("_ROW_._rowid_ > :cursor".to_string()) } - - pk_order = OrderPrecedent::Ascending; - } - - match pk_order { - OrderPrecedent::Descending => Some("_ROW_._rowid_ < :cursor".to_string()), - OrderPrecedent::Ascending => Some("_ROW_._rowid_ > :cursor".to_string()), + // Descending: + _ => Some("_ROW_._rowid_ < :cursor".to_string()), } } else { None }; - let order_clause = order.map_or_else( + let order_clause = order.as_ref().map_or_else( || fmt_order(&pk_column.name, OrderPrecedent::Descending), - |order| { - order - .columns - .into_iter() - .map(|(col, ord)| fmt_order(&col, ord)) + |o| { + o.columns + .iter() + .map(|(col, ord)| fmt_order(col, ord.clone())) .join(",") }, ); @@ -265,7 +262,7 @@ pub async fn list_records_handler( })); } - let cursor: Option = if is_table { + let cursor: Option = if supports_cursor { // The SQL query template returns thw row id as the last column. let value = &last_row[last_row.len() - 1]; let Value::Integer(rowid) = value else { diff --git a/crates/core/src/records/subscribe.rs b/crates/core/src/records/subscribe.rs index 43eda234..47103a5a 100644 --- a/crates/core/src/records/subscribe.rs +++ b/crates/core/src/records/subscribe.rs @@ -823,7 +823,8 @@ struct SubscriptionQuery { impl SubscriptionQuery { fn parse(query: &str) -> Result { - // NOTE: We rely on form-encoding to properly parse ampersands, e.g.: `filter[col0]=a&b%filter[col1]=c`. + // NOTE: We rely on form-encoding to properly parse ampersands, e.g.: + // `filter[col0]=a&b%filter[col1]=c`. let qs = serde_qs::Config::new().max_depth(9).use_form_encoding(true); return qs .deserialize_bytes::(query.as_bytes())