diff --git a/backend/.sqlx/query-e7c8ccc5f1955cae39856baacafb7ce6482125e720fb8f53e3e9e749a69e08bd.json b/backend/.sqlx/query-ea6d40329406aafdd3087e9648760df03b8bd9a4cda44ddeb10462ec960c7547.json similarity index 61% rename from backend/.sqlx/query-e7c8ccc5f1955cae39856baacafb7ce6482125e720fb8f53e3e9e749a69e08bd.json rename to backend/.sqlx/query-ea6d40329406aafdd3087e9648760df03b8bd9a4cda44ddeb10462ec960c7547.json index 3262c18b6f..5b1f36fc2e 100644 --- a/backend/.sqlx/query-e7c8ccc5f1955cae39856baacafb7ce6482125e720fb8f53e3e9e749a69e08bd.json +++ b/backend/.sqlx/query-ea6d40329406aafdd3087e9648760df03b8bd9a4cda44ddeb10462ec960c7547.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg,\n av.created_by as created_by, av.created_at as created_at\n FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version\n WHERE a.workspace_id = $1 AND a.path = $2\n ORDER BY array_position(a.versions, av.id) DESC NULLS LAST, av.id DESC\n LIMIT $3 OFFSET $4", + "query": "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg,\n av.created_by as created_by, av.created_at as created_at\n FROM app a\n JOIN LATERAL (\n SELECT v.id, v.ord FROM unnest(a.versions) WITH ORDINALITY AS v(id, ord)\n ORDER BY v.ord DESC\n LIMIT $3 OFFSET $4\n ) page ON TRUE\n JOIN app_version av ON av.id = page.id AND av.app_id = a.id\n LEFT JOIN deployment_metadata dm ON av.id = dm.app_version\n WHERE a.workspace_id = $1 AND a.path = $2\n ORDER BY page.ord DESC", "describe": { "columns": [ { @@ -45,5 +45,5 @@ false ] }, - "hash": "e7c8ccc5f1955cae39856baacafb7ce6482125e720fb8f53e3e9e749a69e08bd" + "hash": "ea6d40329406aafdd3087e9648760df03b8bd9a4cda44ddeb10462ec960c7547" } diff --git a/backend/tests/app_version_order.rs b/backend/tests/app_version_order.rs index c13893c500..2940db058d 100644 --- a/backend/tests/app_version_order.rs +++ b/backend/tests/app_version_order.rs @@ -94,7 +94,66 @@ async fn test_app_head_follows_the_append_order_not_the_timestamps( assert_eq!( listed, vec![second, first], - "the picker numbers the history by deployed order, so it leads with the head" + "the history lists in deployed order, so it leads with the head" + ); + + // Enough versions that a page cannot hold them, so "asked for nothing" and "asked for + // a page" are visibly different answers. + let mut appended = vec![second, first]; + for _ in 0..24 { + let extra: i64 = sqlx::query_scalar( + "INSERT INTO app_version (app_id, value, created_by, created_at, raw_app) + SELECT app_id, value, 'bulk', created_at, raw_app FROM app_version WHERE id = $1 + RETURNING id", + ) + .bind(first) + .fetch_one(&db) + .await?; + sqlx::query( + "UPDATE app SET versions = array_append(versions, $1::bigint) + WHERE workspace_id = 'test-workspace' AND path = 'u/test-user/order_app'", + ) + .bind(extra) + .execute(&db) + .await?; + appended.insert(0, extra); + } + + let versions_at = |query: &str| { + let url = format!("{ws}/apps/history/p/u/test-user/order_app{query}"); + let client = client.clone(); + async move { + let rows: Vec = client + .get(url) + .header("Authorization", format!("Bearer {TOKEN}")) + .send() + .await? + .json() + .await?; + Ok::<_, anyhow::Error>( + rows.iter() + .map(|v| v["version"].as_i64().unwrap()) + .collect::>(), + ) + } + }; + + // The deployment-history panel and the CLI read this endpoint without paging, so + // asking for no page has to keep answering with the whole history. + assert_eq!( + versions_at("").await?, + appended, + "an unpaginated request still answers whole" + ); + assert_eq!( + versions_at("?per_page=10").await?, + appended[..10], + "a page holds what was asked for, newest first" + ); + assert_eq!( + versions_at("?per_page=10&page=2").await?, + appended[10..20], + "the next page carries on where the first left off, skipping nothing" ); Ok(()) diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 71fbb3d5e2..fda10cd128 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -52,8 +52,8 @@ use windmill_common::{ schedule::Schedule, triggers::MovedNativeTrigger, utils::{ - http_get_from_hub, not_found_if_none, paginate, paginate_with_default, Pagination, - RunnableKind, StripPath, HISTORY_PER_PAGE, + http_get_from_hub, not_found_if_none, paginate, paginate_without_limits, Pagination, + RunnableKind, StripPath, }, }; use windmill_dep_map::scoped_dependency_map::ScopedDependencyMap; @@ -935,7 +935,9 @@ async fn get_flow_history( ) -> JsonResult> { let path = path.to_path(); check_scopes(&authed, || format!("flows:read:{}", path))?; - let (per_page, offset) = paginate_with_default(pagination, HISTORY_PER_PAGE); + // Unasked-for, this listing stays whole: the history panels, the restart picker and + // the CLI all read it without paging. The diff picker asks for a page. + let (per_page, offset) = paginate_without_limits(pagination); let mut tx = user_db.begin(&authed).await?; let flows = sqlx::query_as!( diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index cc98fad55c..665d0315db 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -73,10 +73,7 @@ use windmill_common::{ }, triggers::MovedNativeTrigger, users::username_to_permissioned_as, - utils::{ - not_found_if_none, paginate_with_default, query_elems_from_hub, require_admin, Pagination, - StripPath, HISTORY_PER_PAGE, - }, + utils::{not_found_if_none, query_elems_from_hub, require_admin, Pagination, StripPath}, worker::to_raw_value, HUB_BASE_URL, }; @@ -3095,7 +3092,9 @@ async fn get_script_history( ) -> JsonResult> { let path = path.to_path(); check_scopes(&authed, || format!("scripts:read:{}", path))?; - let (per_page, offset) = paginate_with_default(pagination, HISTORY_PER_PAGE); + // Unasked-for, this listing stays whole: the deployment-history panels, the restart + // picker and the CLI all read it without paging. The diff picker asks for a page. + let (per_page, offset) = paginate_without_limits(pagination); let mut tx = user_db.begin(&authed).await?; let query_result = sqlx::query!( "SELECT s.hash as hash, dm.deployment_msg as deployment_msg, s.created_at as created_at, s.created_by as created_by diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index c4659da170..9b9ee17b1a 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -69,9 +69,8 @@ use windmill_common::{ user_drafts::{overlay_or_draft_only, DraftUserRef, UserDraftItemKind, WithDraftOverlay}, users::username_to_permissioned_as, utils::{ - http_get_from_hub, not_found_if_none, paginate, paginate_with_default, + http_get_from_hub, not_found_if_none, paginate, paginate_without_limits, query_elems_from_hub, require_admin, strip_json_nul, Pagination, RunnableKind, StripPath, - HISTORY_PER_PAGE, }, variables::{build_crypt, build_crypt_with_key_suffix, encrypt}, worker::{to_raw_value, CLOUD_HOSTED}, @@ -1242,26 +1241,35 @@ async fn get_app_history( ) -> JsonResult> { let path = path.to_path(); check_scopes(&authed, || format!("apps:read:{}", &path))?; - let (per_page, offset) = paginate_with_default(pagination, HISTORY_PER_PAGE); + // Unasked-for, this listing stays whole: the deployment-history panel reads it + // without paging. The diff picker asks for a page. + let (per_page, offset) = paginate_without_limits(pagination); let mut tx = user_db.begin(&authed).await?; - // Newest first in the order the versions were deployed, which the picker numbers - // (`v1`, `v2`, …) and reads the head off. That is the position in `app.versions`, - // not `created_at`: the latter is the deploying transaction's start time, so two - // that overlap can carry it in the opposite order from the one they landed in. A - // version absent from the array (restored from trash, copied by a fork) never sat - // in that sequence, so it trails the ones that did. + // Newest first in the order the versions were deployed, which is their position in + // `app.versions` and not `created_at`: the latter is the deploying transaction's + // start time, so two that overlap can carry it in the opposite order from the one + // they landed in. Paging happens inside the array, before the joins, so a page costs + // its own rows rather than every version the path ever had. let query_result = sqlx::query!( "SELECT a.id as app_id, av.id as version_id, dm.deployment_msg as deployment_msg, av.created_by as created_by, av.created_at as created_at - FROM app a LEFT JOIN app_version av ON a.id = av.app_id LEFT JOIN deployment_metadata dm ON av.id = dm.app_version + FROM app a + JOIN LATERAL ( + SELECT v.id, v.ord FROM unnest(a.versions) WITH ORDINALITY AS v(id, ord) + ORDER BY v.ord DESC + LIMIT $3 OFFSET $4 + ) page ON TRUE + JOIN app_version av ON av.id = page.id AND av.app_id = a.id + LEFT JOIN deployment_metadata dm ON av.id = dm.app_version WHERE a.workspace_id = $1 AND a.path = $2 - ORDER BY array_position(a.versions, av.id) DESC NULLS LAST, av.id DESC - LIMIT $3 OFFSET $4", + ORDER BY page.ord DESC", w_id, path, per_page as i64, offset as i64, - ).fetch_all(&mut *tx).await?; + ) + .fetch_all(&mut *tx) + .await?; tx.commit().await?; let result: Vec = query_result diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 56056a58f7..4b93bb89ff 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -479,21 +479,6 @@ pub fn paginate(pagination: Pagination) -> (usize, usize) { (per_page, offset) } -/// What one page of a deploy history holds when the caller asks for no size. The -/// picker that reads these shows a screenful and fetches the next page on demand; -/// a path a pipeline deploys carries far more versions than anyone scrolls. -pub const HISTORY_PER_PAGE: usize = 20; - -/// [`paginate`] for a listing whose unasked-for page should be smaller than the generic -/// default — a version history behind a picker, where a path deployed by CI carries -/// thousands of entries and only the first screenful is ever read. -pub fn paginate_with_default(pagination: Pagination, default_per_page: usize) -> (usize, usize) { - paginate(Pagination { - per_page: Some(pagination.per_page.unwrap_or(default_per_page)), - ..pagination - }) -} - pub fn paginate_without_limits(pagination: Pagination) -> (usize, usize) { let per_page = pagination.per_page.unwrap_or(MAX_PER_PAGE); let offset = (pagination.page.unwrap_or(1).max(1) - 1) * per_page; diff --git a/frontend/src/lib/components/DiffDrawer.svelte b/frontend/src/lib/components/DiffDrawer.svelte index 79f8ce8b56..0958425f61 100644 --- a/frontend/src/lib/components/DiffDrawer.svelte +++ b/frontend/src/lib/components/DiffDrawer.svelte @@ -105,8 +105,9 @@ export function abandonOpening(token: number) { if (token !== openingToken) return openingToken++ - // The version load in flight, if any, belongs to the diff being dropped. + // The version load and the page in flight, if any, belong to the diff being dropped. versionLoadGeneration++ + versionListGeneration++ loadingVersion = false data = undefined diffType = undefined @@ -142,6 +143,11 @@ * generation is stale — a slower earlier pick, or one outlived by a drawer reset — * and neither replaces the diff nor clears the spinner, which `disabled` rides on. */ let versionLoadGeneration = 0 + /** Counted separately from `versionLoadGeneration`: only a diff replacing this one + * invalidates a page in flight. Picking a version while one loads is not a reason to + * drop it — the editor's cursor has already moved past that page, so discarding it + * would skip it until the drawer is reopened. */ + let versionListGeneration = 0 /** Pages of `versions` fetched after the first. Kept beside `data` so a diff swapped in * by a newer opening drops them along with the list they extended. */ let extraVersions: DiffVersionOption[] = $state([]) @@ -158,12 +164,12 @@ async function fetchMoreVersions() { if (!moreLoader || loadingMore) return loadingMore = true - const generation = versionLoadGeneration + const generation = versionListGeneration try { const more = await moreLoader() // A diff swapped in while this ran owns the picker now; appending would splice // one item's history onto another's. - if (generation !== versionLoadGeneration) return + if (generation !== versionListGeneration) return if (more?.length) { extraVersions = [...extraVersions, ...more] } else { @@ -171,7 +177,7 @@ moreLoader = undefined } } catch (e: any) { - if (generation === versionLoadGeneration) { + if (generation === versionListGeneration) { sendUserToast(`Could not load older versions: ${e?.body ?? e?.message ?? e}`, true) } } finally { @@ -294,8 +300,9 @@ moreLoader = loadMoreVersions headLabel = deployedLabel headDeployed = !deployed.draft_only ? prepareDiff(deployed) : undefined - // A load still in flight belongs to the diff being replaced. + // A load or page still in flight belongs to the diff being replaced. versionLoadGeneration++ + versionListGeneration++ loadingVersion = false extraVersions = [] loadingMore = false diff --git a/frontend/src/lib/components/FlowBuilder.svelte b/frontend/src/lib/components/FlowBuilder.svelte index 0ca90b33f6..9fc92252a1 100644 --- a/frontend/src/lib/components/FlowBuilder.svelte +++ b/frontend/src/lib/components/FlowBuilder.svelte @@ -10,6 +10,7 @@ type Job } from '$lib/gen' import { initHistory, redo, undo } from '$lib/history.svelte' + import { VERSION_PAGE_SIZE } from '$lib/components/diff_drawer' import { clearLinkedAgentTools, linkedAgentToolsForScope, @@ -1141,7 +1142,12 @@ const path = userDraftPath || initialPath if (!opWorkspace || !path) return undefined try { - const history = await FlowService.getFlowHistory({ workspace: opWorkspace, path, page }) + const history = await FlowService.getFlowHistory({ + workspace: opWorkspace, + path, + page, + perPage: VERSION_PAGE_SIZE + }) // Head is the version the payload beside this list came from, not whatever the // history now leads with: a deploy landing between the two fetches would // otherwise label the shown (older) value as the latest. diff --git a/frontend/src/lib/components/ScriptBuilder.svelte b/frontend/src/lib/components/ScriptBuilder.svelte index d7cffb672e..e792ce7435 100644 --- a/frontend/src/lib/components/ScriptBuilder.svelte +++ b/frontend/src/lib/components/ScriptBuilder.svelte @@ -13,6 +13,7 @@ WorkerService } from '$lib/gen' import { inferArgs } from '$lib/infer' + import { VERSION_PAGE_SIZE } from '$lib/components/diff_drawer' import { initialCode, canHavePreprocessor, @@ -849,7 +850,8 @@ const history = await ScriptService.getScriptHistoryByPath({ workspace: opWorkspace, path: userDraftPath, - page + page, + perPage: VERSION_PAGE_SIZE }) // The hash identifies the version — it is what the API and the CLI speak — and // who deployed it drops to the subtitle. No ordinal: the list arrives a page at diff --git a/frontend/src/lib/components/diff_drawer.ts b/frontend/src/lib/components/diff_drawer.ts index b7bc248948..c1e7e8980a 100644 --- a/frontend/src/lib/components/diff_drawer.ts +++ b/frontend/src/lib/components/diff_drawer.ts @@ -12,6 +12,11 @@ export type DiffVersionOption = { isHead?: boolean } +/** How many deployed versions the diff picker asks for at a time. The history endpoints + * still answer whole when nobody asks — the panels and the CLI read them that way — so + * this is the picker's own appetite, not their default. */ +export const VERSION_PAGE_SIZE = 20 + export type DiffDrawerDiff = | { mode: 'normal' diff --git a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte index f9d5481591..4bef3b5599 100644 --- a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte @@ -39,6 +39,7 @@ import DeploymentHistory from '../apps/editor/DeploymentHistory.svelte' import Awareness from '$lib/components/Awareness.svelte' import type DiffDrawer from '$lib/components/DiffDrawer.svelte' + import { VERSION_PAGE_SIZE } from '$lib/components/diff_drawer' import EditorHeader from '$lib/components/EditorHeader.svelte' import AutosaveIndicator from '$lib/components/AutosaveIndicator.svelte' @@ -438,7 +439,8 @@ const history = await AppService.getAppHistoryByPath({ workspace: opWorkspace, path: appPath, - page + page, + perPage: VERSION_PAGE_SIZE }) // Head is the version the payload beside this list came from; see FlowBuilder. const head = deployedVersionShown ?? history[0]?.version