mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 08:02:38 +00:00
fix: a history stays whole unless asked to page, and pages inside the version array
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
a2b0d1c925
commit
98d59ce4b6
+2
-2
@@ -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"
|
||||
}
|
||||
@@ -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<serde_json::Value> = 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::<Vec<_>>(),
|
||||
)
|
||||
}
|
||||
};
|
||||
|
||||
// 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(())
|
||||
|
||||
@@ -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<Vec<FlowVersion>> {
|
||||
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!(
|
||||
|
||||
@@ -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<Vec<ScriptHistory>> {
|
||||
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
|
||||
|
||||
@@ -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<Vec<AppHistory>> {
|
||||
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<AppHistory> = query_result
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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'
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user