mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-20 00:02:19 +00:00
fix(drafts): surface draft-only apps and path-prefixed draft-only rows
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
centdix
+3
-2
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at\n FROM draft\n WHERE workspace_id = $1\n AND typ = 'script'\n AND email = $2\n AND NOT EXISTS (\n SELECT 1 FROM script s\n WHERE s.workspace_id = draft.workspace_id\n AND s.path = draft.path\n )",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at\n FROM draft\n WHERE workspace_id = $1\n AND typ = 'script'\n AND email = $2\n AND ($3::text IS NULL OR path LIKE $3 || '%')\n AND NOT EXISTS (\n SELECT 1 FROM script s\n WHERE s.workspace_id = draft.workspace_id\n AND s.path = draft.path\n )",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
@@ -21,6 +21,7 @@
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
@@ -31,5 +32,5 @@
|
||||
false
|
||||
]
|
||||
},
|
||||
"hash": "56741ef2a510917f6c460968117b26e3ef9171f478e7ab3bd68eb09663b393c4"
|
||||
"hash": "697c8c8794a4ad6aece92810319dc37404e9d6bb598842d3c7f846c2e50457f0"
|
||||
}
|
||||
+3
-2
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at,\n typ::text as \"typ!\"\n FROM draft\n WHERE workspace_id = $1\n AND typ IN ('app', 'raw_app')\n AND email = $2\n AND NOT EXISTS (\n SELECT 1 FROM app a\n WHERE a.workspace_id = draft.workspace_id\n AND a.path = draft.path\n )",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at,\n typ::text as \"typ!\"\n FROM draft\n WHERE workspace_id = $1\n AND typ IN ('app', 'raw_app')\n AND email = $2\n AND ($3::text IS NULL OR path LIKE $3 || '%')\n AND NOT EXISTS (\n SELECT 1 FROM app a\n WHERE a.workspace_id = draft.workspace_id\n AND a.path = draft.path\n )",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
@@ -26,6 +26,7 @@
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
@@ -37,5 +38,5 @@
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "e39a6474d84e1bc10a33351b6970ade7b6eb611beed13570aeda41a37efa927d"
|
||||
"hash": "86a7ff7c5f178844007b6e66ca0fe47c26034d500c35dad5442bdc14bf8d1aa8"
|
||||
}
|
||||
+3
-2
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at\n FROM draft\n WHERE workspace_id = $1\n AND typ = 'flow'\n AND email = $2\n AND NOT EXISTS (\n SELECT 1 FROM flow f\n WHERE f.workspace_id = draft.workspace_id\n AND f.path = draft.path\n )",
|
||||
"query": "SELECT path,\n value as \"value!: sqlx::types::Json<Box<serde_json::value::RawValue>>\",\n created_at\n FROM draft\n WHERE workspace_id = $1\n AND typ = 'flow'\n AND email = $2\n AND ($3::text IS NULL OR path LIKE $3 || '%')\n AND NOT EXISTS (\n SELECT 1 FROM flow f\n WHERE f.workspace_id = draft.workspace_id\n AND f.path = draft.path\n )",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
@@ -21,6 +21,7 @@
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
@@ -31,5 +32,5 @@
|
||||
false
|
||||
]
|
||||
},
|
||||
"hash": "bf945c34c18a9fe5f0af8f54042df39178f5111119d50f2975dd1180502c0c52"
|
||||
"hash": "f8e8761da49f33e9cf7d73586757e7cc4515081955b8850bebc4ee817819e71a"
|
||||
}
|
||||
@@ -233,12 +233,12 @@ async fn list_flows(
|
||||
//
|
||||
// Concatenated after the deployed page so the home page surfaces
|
||||
// them too. Fields not in the draft JSON fall back to sensible
|
||||
// defaults. Skipped when filters narrow the list or we're past
|
||||
// page 0 — keeps pagination semantics clean.
|
||||
// defaults. `path_start` is honored in-query (so prefix listings
|
||||
// still include draft-only rows); other narrowing filters or pages
|
||||
// past 0 skip the append to keep pagination semantics clean.
|
||||
if lq.include_draft_only.unwrap_or(false)
|
||||
&& !authed.is_operator
|
||||
&& offset == 0
|
||||
&& lq.path_start.is_none()
|
||||
&& lq.path_exact.is_none()
|
||||
&& lq.edited_by.is_none()
|
||||
&& lq.dedicated_worker.is_none()
|
||||
@@ -254,6 +254,7 @@ async fn list_flows(
|
||||
WHERE workspace_id = $1
|
||||
AND typ = 'flow'
|
||||
AND email = $2
|
||||
AND ($3::text IS NULL OR path LIKE $3 || '%')
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM flow f
|
||||
WHERE f.workspace_id = draft.workspace_id
|
||||
@@ -261,6 +262,7 @@ async fn list_flows(
|
||||
)"#,
|
||||
&w_id,
|
||||
&authed.email,
|
||||
lq.path_start.as_deref(),
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
|
||||
@@ -343,12 +343,12 @@ async fn list_scripts(
|
||||
//
|
||||
// Concatenated after the deployed page so the home page surfaces
|
||||
// them too. Fields not in the draft JSON fall back to sensible
|
||||
// defaults. Skipped when filters narrow the list or we're past
|
||||
// page 0 — keeps pagination semantics clean.
|
||||
// defaults. `path_start` is honored in-query (so prefix listings
|
||||
// still include draft-only rows); other narrowing filters or pages
|
||||
// past 0 skip the append to keep pagination semantics clean.
|
||||
if lq.include_draft_only.unwrap_or(false)
|
||||
&& !authed.is_operator
|
||||
&& offset == 0
|
||||
&& lq.path_start.is_none()
|
||||
&& lq.path_exact.is_none()
|
||||
&& lq.created_by.is_none()
|
||||
&& lq.first_parent_hash.is_none()
|
||||
@@ -369,6 +369,7 @@ async fn list_scripts(
|
||||
WHERE workspace_id = $1
|
||||
AND typ = 'script'
|
||||
AND email = $2
|
||||
AND ($3::text IS NULL OR path LIKE $3 || '%')
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM script s
|
||||
WHERE s.workspace_id = draft.workspace_id
|
||||
@@ -376,6 +377,7 @@ async fn list_scripts(
|
||||
)"#,
|
||||
&w_id,
|
||||
&authed.email,
|
||||
lq.path_start.as_deref(),
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
|
||||
@@ -458,11 +458,12 @@ async fn list_apps(
|
||||
//
|
||||
// Concatenated after the deployed page so the home page surfaces
|
||||
// them too. Fields not in the draft JSON fall back to defaults.
|
||||
// Skipped when filters narrow the list or we're past page 0.
|
||||
// `path_start` is honored in-query (so prefix listings still include
|
||||
// draft-only rows); other narrowing filters or pages past 0 skip the
|
||||
// append to keep pagination clean.
|
||||
if lq.include_draft_only.unwrap_or(false)
|
||||
&& !authed.is_operator
|
||||
&& offset == 0
|
||||
&& lq.path_start.is_none()
|
||||
&& lq.path_exact.is_none()
|
||||
&& lq.label.is_none()
|
||||
&& !lq.starred_only.unwrap_or(false)
|
||||
@@ -476,6 +477,7 @@ async fn list_apps(
|
||||
WHERE workspace_id = $1
|
||||
AND typ IN ('app', 'raw_app')
|
||||
AND email = $2
|
||||
AND ($3::text IS NULL OR path LIKE $3 || '%')
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM app a
|
||||
WHERE a.workspace_id = draft.workspace_id
|
||||
@@ -483,6 +485,7 @@ async fn list_apps(
|
||||
)"#,
|
||||
&w_id,
|
||||
&authed.email,
|
||||
lq.path_start.as_deref(),
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
|
||||
@@ -139,13 +139,19 @@ row:
|
||||
- `draft_only` — the item exists only as a draft (never deployed)
|
||||
- `is_draft` — a deployed item that has a pending draft for this user
|
||||
|
||||
(draft-only rows also set `is_draft = true`). `listWorkspaceItems` *already* called those
|
||||
endpoints with `includeDraftOnly: true` — it just hardcoded `isDraft: false`. So the fix
|
||||
is to read the flags: no new endpoint, no `DraftService.listDrafts`, no `typ`→type mapper.
|
||||
(draft-only rows also set `is_draft = true`). `listWorkspaceItems` *already* called the
|
||||
script/flow endpoints with `includeDraftOnly: true` — it just hardcoded `isDraft: false`.
|
||||
So the core fix is to read the flags: no new endpoint, no `DraftService.listDrafts`, no
|
||||
`typ`→type mapper. Two follow-ups from review were also needed: the **app** call was
|
||||
missing `includeDraftOnly` (issue #1), and listing under a **`path_prefix`** needed a
|
||||
small backend change so draft-only rows are filtered by prefix instead of dropped (issue
|
||||
#2 / option (c) — see below).
|
||||
|
||||
### What was implemented
|
||||
|
||||
- **`core.ts` `listWorkspaceItems`** — `isDraft` is now derived from the flags:
|
||||
- **`core.ts` `listWorkspaceItems`** — all three list calls pass `includeDraftOnly: true`
|
||||
(the app call previously omitted it — **issue #1 fix**), and `isDraft` is derived from
|
||||
the returned flags:
|
||||
- script/flow: `draft_only === true || is_draft === true` (both fields on the list row)
|
||||
- app: `is_draft === true` (`ListableApp` exposes only `is_draft`; draft-only apps set it)
|
||||
- **`core.ts` list tool** — dropped the `listGlobalDrafts` merge. Script/flow/app drafts
|
||||
@@ -168,6 +174,19 @@ is to read the flags: no new endpoint, no `DraftService.listDrafts`, no `typ`→
|
||||
- **`global_drafts/+page.svelte`** (dev inspector) — now calls `DraftService.listDrafts`
|
||||
directly (raw `{ path, typ, saved_at }`) and deletes via `DraftService.saveDraft`
|
||||
(`value: null`). Self-contained; no dependency on the adapter.
|
||||
- **Backend — `scripts.rs` / `flows.rs` / `apps.rs` `list_*`** (**issue #2, option (c)**):
|
||||
the draft-only append no longer bails when `path_start` is set; instead the draft-only
|
||||
query filters by it (`AND ($N::text IS NULL OR path LIKE $N || '%')`, mirroring the
|
||||
deployed query's `and_where_like_left`). Other narrowing filters (`path_exact`,
|
||||
`created_by`, `label`, languages, pages past 0) still skip the append. sqlx offline cache
|
||||
regenerated for the three changed queries.
|
||||
|
||||
> **⚠️ Reviewer — please confirm (c):** removing the `path_start.is_none()` guard from the
|
||||
> draft-only append is the chosen fix for issue #2. The guard was presumably there so
|
||||
> pickers/selectors get a deployed-only listing; the global-chat list tool needs draft-only
|
||||
> rows under a prefix, and honoring `path_start` in-query (rather than dropping draft-only)
|
||||
> is the least-surprising behavior. If a caller relies on "prefix query ⇒ no synthesized
|
||||
> draft-only rows", this changes that. Flagging for sign-off.
|
||||
|
||||
### Known limitation (intentional, awaiting backend)
|
||||
|
||||
@@ -180,11 +199,13 @@ Lift it by adding `includeDraftOnly` (email-scoped) to those endpoints.
|
||||
|
||||
- Backend confirmed live: a seeded draft-only script returns `draft_only: true,
|
||||
is_draft: true` from `listScripts?include_draft_only=true`, scoped to the authed email.
|
||||
- Unit (`core.test.ts`): `flags backend draft scripts and forwards path_prefix + limit`
|
||||
passes; the live-editor **list** assertions in `lists … the live script/flow editor
|
||||
draft` pass too. Those two tests then fail later in their *edit/write* half on the
|
||||
unrelated read-after-write gap (`getXByPath mock not configured` — see §3/§8). Net
|
||||
suite delta: **29 → 28 failures, no regressions.**
|
||||
After the (c) change, the same query **with** `path_start` set returns the draft-only row
|
||||
when it matches the prefix (and omits it otherwise) — verified via the API.
|
||||
- Unit (`core.test.ts`): `flags backend draft scripts and forwards path_prefix + limit` and
|
||||
`requests draft-only apps and flags them via is_draft` (issue #1 guard) pass; the
|
||||
live-editor **list** assertions in `lists … the live script/flow editor draft` pass too.
|
||||
Those two live-editor tests then fail later in their *edit/write* half on the unrelated
|
||||
read-after-write gap (`getXByPath mock not configured` — see §3/§8). No regressions.
|
||||
- E2E (not yet run): with `wm_dev_global_ai` enabled, create a draft script via the global
|
||||
mode, reload, confirm `list_workspace_items` flags it `isDraft: true`.
|
||||
|
||||
|
||||
@@ -444,7 +444,9 @@ describe('global AI tools', () => {
|
||||
// Post #9351 the list tool sources script drafts from `listScripts`
|
||||
// (`includeDraftOnly` + the `draft_only`/`is_draft` flags), not a local
|
||||
// store — so path_prefix (pathStart) and limit (perPage) are forwarded to
|
||||
// the backend query, which does the filtering.
|
||||
// the backend query, which does the filtering. The backend now filters
|
||||
// draft-only rows by `path_start` too (the (c) fix in scripts/flows/apps),
|
||||
// so a draft-only row under the prefix is returned and flagged here.
|
||||
vi.mocked(ScriptService.listScripts).mockResolvedValueOnce([
|
||||
{ path: 'f/matching/inside', summary: 'Inside draft', language: 'bun', draft_only: true }
|
||||
] as any)
|
||||
@@ -467,6 +469,25 @@ describe('global AI tools', () => {
|
||||
])
|
||||
})
|
||||
|
||||
it('requests draft-only apps and flags them via is_draft', async () => {
|
||||
// Regression guard: `listApps` must be called with `includeDraftOnly`
|
||||
// (the app call previously omitted it, so DB-only app drafts were missed),
|
||||
// and a draft row maps to `isDraft: true` (draft-only apps carry
|
||||
// `is_draft === true`).
|
||||
vi.mocked(AppService.listApps).mockResolvedValueOnce([
|
||||
{ path: 'f/apps/inflight', summary: 'In-flight app', draft_only: true, is_draft: true }
|
||||
] as any)
|
||||
|
||||
const raw = await callGlobalTool('list_workspace_items', { types: ['app'] })
|
||||
|
||||
expect(AppService.listApps).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ includeDraftOnly: true })
|
||||
)
|
||||
expect(JSON.parse(raw)).toEqual([
|
||||
expect.objectContaining({ type: 'app', path: 'f/apps/inflight', isDraft: true })
|
||||
])
|
||||
})
|
||||
|
||||
it('lists and edits the live script editor draft through its effective path', async () => {
|
||||
UserDraft.save(
|
||||
'script',
|
||||
|
||||
@@ -1277,7 +1277,8 @@ async function listWorkspaceItems(
|
||||
const apps = await AppService.listApps({
|
||||
workspace,
|
||||
pathStart: pathPrefix,
|
||||
perPage
|
||||
perPage,
|
||||
includeDraftOnly: true
|
||||
})
|
||||
// Draft-only apps also carry `is_draft === true`, so it covers both
|
||||
// draft-only and deployed-with-draft (ListableApp exposes only `is_draft`).
|
||||
|
||||
Reference in New Issue
Block a user