mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 08:02:47 +00:00
fix: an unwritten subscription is fixed on its script only, by removing it
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
7a3bc67c59
commit
0024e623c2
@@ -40,16 +40,13 @@ webhook, a queue…): the add node's, and the canvas's right-click one.
|
||||
trigger: Snippet<[{ open: boolean }]>
|
||||
/** Opens the menu each time it changes. */
|
||||
openSignal?: number
|
||||
/** With `openSignal`: the output the new script writes. */
|
||||
presetOutput?: { kind: 'ducklake' | 'datatable'; store: string; table: string }
|
||||
}
|
||||
let {
|
||||
onAddPipelineScript,
|
||||
pathPrefix,
|
||||
defaultPathSuffix,
|
||||
trigger,
|
||||
openSignal,
|
||||
presetOutput
|
||||
openSignal
|
||||
}: Props = $props()
|
||||
|
||||
function handlePick(pick: PipelineInsertPick) {
|
||||
@@ -149,6 +146,5 @@ webhook, a queue…): the add node's, and the canvas's right-click one.
|
||||
{defaultPathSuffix}
|
||||
onPick={handlePick}
|
||||
{openSignal}
|
||||
{presetOutput}
|
||||
{trigger}
|
||||
/>
|
||||
|
||||
@@ -365,9 +365,6 @@
|
||||
})
|
||||
let addMenuAt = $state<{ x: number; y: number }>({ x: 0, y: 0 })
|
||||
let addMenuSignal = $state(0)
|
||||
let addMenuPreset = $state<
|
||||
{ kind: 'ducklake' | 'datatable'; store: string; table: string } | undefined
|
||||
>(undefined)
|
||||
|
||||
// The schedule wizard a Fix action opens for an existing script, at the pill.
|
||||
let scheduleWizard = $state<
|
||||
@@ -391,11 +388,6 @@
|
||||
if (a.scheduleFor) {
|
||||
scheduleWizard = { ...a.scheduleFor, at: anchor }
|
||||
scheduleWizardSignal++
|
||||
} else if (a.addWriter) {
|
||||
const [store, ...rest] = a.addWriter.path.split('/')
|
||||
addMenuAt = anchor
|
||||
addMenuPreset = { kind: a.addWriter.kind, store, table: rest.join('/') }
|
||||
addMenuSignal++
|
||||
} else a.run?.()
|
||||
}
|
||||
}))
|
||||
@@ -1865,7 +1857,6 @@
|
||||
onclick={() => {
|
||||
if (paneMenu) addMenuAt = paneMenu
|
||||
paneMenu = undefined
|
||||
addMenuPreset = undefined
|
||||
addMenuSignal++
|
||||
}}
|
||||
>
|
||||
@@ -1937,7 +1928,6 @@
|
||||
{pathPrefix}
|
||||
{defaultPathSuffix}
|
||||
openSignal={addMenuSignal}
|
||||
presetOutput={addMenuPreset}
|
||||
>
|
||||
{#snippet trigger()}
|
||||
<span class="block w-0 h-0" aria-hidden="true"></span>
|
||||
|
||||
@@ -219,7 +219,8 @@
|
||||
}
|
||||
}
|
||||
|
||||
let showAdd = $derived(data.onAddScript != undefined)
|
||||
// A red asset needs fixing where its error comes from, not a downstream step.
|
||||
let showAdd = $derived(data.onAddScript != undefined && !data.error)
|
||||
let menuItems: Item[] = $derived(
|
||||
data.onDeleteUpstream
|
||||
? [
|
||||
|
||||
@@ -16,15 +16,14 @@ red fill already says something is wrong.
|
||||
export type FixExplainer = Array<string | { text: string; onClick: () => void }>
|
||||
export type NodeFix = { explainer: FixExplainer; actions: NodeFixAction[] }
|
||||
|
||||
/** A fix as its supplier declares it: an action, or opening the add menu with
|
||||
* the new script's output preset (which the canvas owns). */
|
||||
/** A fix as its supplier declares it: an action, or opening the schedule
|
||||
* wizard (which the canvas owns). */
|
||||
export type NodeFixSpec = {
|
||||
explainer: FixExplainer
|
||||
actions: Array<{
|
||||
label: string
|
||||
detail?: string
|
||||
run?: () => void
|
||||
addWriter?: { kind: 'ducklake' | 'datatable'; path: string }
|
||||
/** Opens the schedule wizard for this script; its confirm calls `onSchedule`. */
|
||||
scheduleFor?: {
|
||||
script: string
|
||||
|
||||
@@ -87,8 +87,6 @@
|
||||
placement?: 'bottom' | 'top' | 'left' | 'right'
|
||||
/** Opens the menu each time it changes, for an entry point other than the trigger. */
|
||||
openSignal?: number
|
||||
/** With `openSignal`: the new script's output, already picked and named. */
|
||||
presetOutput?: { kind: 'ducklake' | 'datatable'; store: string; table: string }
|
||||
/** With `openSignal`: only the schedule step, for this existing script; its
|
||||
* confirm hands the schedule to `onSchedule` instead of creating a script. */
|
||||
scheduleFor?: string
|
||||
@@ -104,7 +102,6 @@
|
||||
defaultPathSuffix,
|
||||
onPick,
|
||||
openSignal,
|
||||
presetOutput,
|
||||
scheduleFor,
|
||||
onSchedule
|
||||
}: Props = $props()
|
||||
@@ -122,21 +119,9 @@
|
||||
})
|
||||
let selected = $state(buildEmptySelected())
|
||||
let menuOpen = $state(false)
|
||||
// Opened for a given output: the output is not the user's to pick.
|
||||
let outputLocked = $state(false)
|
||||
$effect(() => {
|
||||
if (!openSignal) return
|
||||
untrack(() => {
|
||||
outputLocked = !!presetOutput
|
||||
if (presetOutput) {
|
||||
resetWizard()
|
||||
selected.outputId = presetOutput.kind
|
||||
config.asset = { ...presetOutput }
|
||||
config.tableEdited = true
|
||||
// Named after what it writes, in the characters a script path allows.
|
||||
const name = presetOutput.table.replace(/[^\w-]+/g, '_').replace(/^_+|_+$/g, '')
|
||||
if (name) selected.scriptPath = name
|
||||
}
|
||||
if (scheduleFor) {
|
||||
resetWizard()
|
||||
selected.triggerId = 'schedule'
|
||||
@@ -340,14 +325,7 @@
|
||||
contentClasses={twMerge(
|
||||
'p-0 bg-surface overflow-hidden relative transition-height',
|
||||
// The first step's columns (w-56 + w-48 + w-80), held across steps.
|
||||
// Without the output column when the output is preset (w-80 fewer).
|
||||
singleKind
|
||||
? outputLocked
|
||||
? 'w-[12rem]'
|
||||
: 'w-[32rem]'
|
||||
: outputLocked
|
||||
? 'w-[26rem]'
|
||||
: 'w-[46rem]',
|
||||
singleKind ? 'w-[32rem]' : 'w-[46rem]',
|
||||
currentConfigStep === 'schedule'
|
||||
? 'h-[27rem]'
|
||||
: currentConfigStep === 'asset'
|
||||
@@ -448,9 +426,7 @@
|
||||
'flex flex-col gap-1 p-2 overflow-auto transition-opacity w-48',
|
||||
selected.triggerId ? '' : 'opacity-20'
|
||||
)}
|
||||
{@attach arrowTabNav({
|
||||
onKeyDown: selectAndAdvanceTo(() => (outputLocked ? pathEl : outputEl))
|
||||
})}
|
||||
{@attach arrowTabNav({ onKeyDown: selectAndAdvanceTo(() => outputEl) })}
|
||||
>
|
||||
<div class="text-2xs font-normal text-secondary ml-2 mb-1">Language</div>
|
||||
{#each languages as l}
|
||||
@@ -460,9 +436,6 @@
|
||||
unifiedSize="sm"
|
||||
btnClasses="justify-start"
|
||||
selected={isSelected}
|
||||
disabled={outputLocked &&
|
||||
!!selected.outputId &&
|
||||
!compatibleOutputKinds(l.lang).includes(selected.outputId)}
|
||||
onClick={() => {
|
||||
selected.language = l.lang
|
||||
const _compatibleOutputKinds = compatibleOutputKinds(l.lang)
|
||||
@@ -477,36 +450,34 @@
|
||||
{/each}
|
||||
</div>
|
||||
|
||||
{#if !outputLocked}
|
||||
<div
|
||||
bind:this={outputEl}
|
||||
class={twMerge(
|
||||
'flex flex-col gap-1 p-2 grow w-80 overflow-auto transition-opacity',
|
||||
selected.triggerId && selected.language ? '' : 'opacity-20'
|
||||
)}
|
||||
{@attach arrowTabNav({ onKeyDown: selectAndAdvanceTo(() => pathEl, { timeout: 50 }) })}
|
||||
>
|
||||
<div class="text-2xs font-normal text-secondary ml-2 mb-1">Output asset</div>
|
||||
{#each visibleOutputKinds.length ? visibleOutputKinds : PIPELINE_OUTPUT_KINDS as k}
|
||||
{@const isSelected = selected.outputId === k.id}
|
||||
<Button variant="subtle" selected={isSelected} onClick={() => (selected.outputId = k.id)}>
|
||||
<span class="flex flex-col items-start flex-1 min-w-0 text-left">
|
||||
<span class="text-xs font-normal leading-tight">{k.label}</span>
|
||||
{#if k.description}
|
||||
<span
|
||||
class={twMerge(
|
||||
'text-2xs font-normal leading-snug mt-0.5',
|
||||
isSelected ? 'text-accent/80' : 'text-hint'
|
||||
)}
|
||||
>
|
||||
{k.description}
|
||||
</span>
|
||||
{/if}
|
||||
</span>
|
||||
</Button>
|
||||
{/each}
|
||||
</div>
|
||||
{/if}
|
||||
<div
|
||||
bind:this={outputEl}
|
||||
class={twMerge(
|
||||
'flex flex-col gap-1 p-2 grow w-80 overflow-auto transition-opacity',
|
||||
selected.triggerId && selected.language ? '' : 'opacity-20'
|
||||
)}
|
||||
{@attach arrowTabNav({ onKeyDown: selectAndAdvanceTo(() => pathEl, { timeout: 50 }) })}
|
||||
>
|
||||
<div class="text-2xs font-normal text-secondary ml-2 mb-1">Output asset</div>
|
||||
{#each visibleOutputKinds.length ? visibleOutputKinds : PIPELINE_OUTPUT_KINDS as k}
|
||||
{@const isSelected = selected.outputId === k.id}
|
||||
<Button variant="subtle" selected={isSelected} onClick={() => (selected.outputId = k.id)}>
|
||||
<span class="flex flex-col items-start flex-1 min-w-0 text-left">
|
||||
<span class="text-xs font-normal leading-tight">{k.label}</span>
|
||||
{#if k.description}
|
||||
<span
|
||||
class={twMerge(
|
||||
'text-2xs font-normal leading-snug mt-0.5',
|
||||
isSelected ? 'text-accent/80' : 'text-hint'
|
||||
)}
|
||||
>
|
||||
{k.description}
|
||||
</span>
|
||||
{/if}
|
||||
</span>
|
||||
</Button>
|
||||
{/each}
|
||||
</div>
|
||||
{/snippet}
|
||||
|
||||
{#snippet bottomSection(close: () => void)}
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
addTriggerDirective,
|
||||
listenToAsset,
|
||||
removeTriggerDirective,
|
||||
scheduleInsteadOfAsset,
|
||||
stopListeningToAsset
|
||||
} from './pipelineAnnotationEdits'
|
||||
|
||||
@@ -42,10 +42,9 @@ describe('pipeline annotation edits', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('swaps an asset subscription for a schedule, and drops a trigger kind', () => {
|
||||
const ts = '// pipeline\n// on datatable://main/orders\nexport function main() {}'
|
||||
expect(scheduleInsteadOfAsset(ts, table, false)).toBe(
|
||||
'// pipeline\n// on schedule\nexport function main() {}'
|
||||
it('adds a trigger kind once, and drops it', () => {
|
||||
expect(addTriggerDirective('// pipeline\n// on webhook\nx()', 'webhook')).toBe(
|
||||
'// pipeline\n// on webhook\nx()'
|
||||
)
|
||||
expect(removeTriggerDirective('// pipeline\n// on kafka\nx()', 'kafka')).toBe(
|
||||
'// pipeline\nx()'
|
||||
|
||||
@@ -113,16 +113,6 @@ export function stopListeningToAsset(
|
||||
return out
|
||||
}
|
||||
|
||||
/** The script runs on a schedule rather than after writes to the asset. */
|
||||
export function scheduleInsteadOfAsset(
|
||||
content: string,
|
||||
asset: { kind: AssetKind; path: string },
|
||||
reads: boolean
|
||||
): string {
|
||||
const out = stopListeningToAsset(content, asset, reads)
|
||||
return hasDirective(out, onTrigger('schedule')) ? out : addDirective(out, 'on schedule')
|
||||
}
|
||||
|
||||
/** Adds a `// on <trigger kind>` line (`schedule`, `webhook`, …) unless it has one. */
|
||||
export function addTriggerDirective(content: string, kind: string): string {
|
||||
return hasDirective(content, onTrigger(kind)) ? content : addDirective(content, `on ${kind}`)
|
||||
|
||||
@@ -105,7 +105,6 @@
|
||||
assetRef,
|
||||
listenToAsset,
|
||||
removeTriggerDirective,
|
||||
scheduleInsteadOfAsset,
|
||||
stopListeningChangeText,
|
||||
stopListeningToAsset
|
||||
} from '$lib/components/assets/AssetGraph/pipelineAnnotationEdits'
|
||||
@@ -1358,8 +1357,6 @@
|
||||
}
|
||||
|
||||
const scriptName = (p: string) => p.split('/').pop() ?? p
|
||||
const isTableKind = (k: AssetKind): k is 'ducklake' | 'datatable' =>
|
||||
k === 'ducklake' || k === 'datatable'
|
||||
|
||||
const readsAsset = (script: string, asset: { kind: AssetKind; path: string }) =>
|
||||
shownGraph.edges.some(
|
||||
@@ -1385,77 +1382,9 @@
|
||||
onClick: () => selectAsset(asset)
|
||||
})
|
||||
|
||||
/** The fixes for an asset's unwritten subscription, from either end of it. */
|
||||
function subscriptionFixes(
|
||||
asset: { kind: AssetKind; path: string },
|
||||
subscribers: string[]
|
||||
): NodeFixSpec['actions'] {
|
||||
const one = subscribers.length === 1 ? subscribers[0] : undefined
|
||||
const reads = subscribers.some((s) => readsAsset(s, asset))
|
||||
const actions: NodeFixSpec['actions'] = []
|
||||
if (isTableKind(asset.kind)) {
|
||||
actions.push({
|
||||
label: 'Add a script that writes it',
|
||||
detail: `Opens the new-script menu with ${assetRef(asset)} as its output.`,
|
||||
addWriter: { kind: asset.kind, path: asset.path }
|
||||
})
|
||||
}
|
||||
if (one) {
|
||||
actions.push({
|
||||
label: `Run ${scriptName(one)} on a schedule instead`,
|
||||
detail: `Opens the schedule setup. On create: ${stopListeningChangeText(asset, reads).replace(/^R/, 'r')} and adds \`on schedule\` in ${one}, with the schedule as a draft.`,
|
||||
scheduleFor: {
|
||||
script: one,
|
||||
onSchedule: async (schedule) => {
|
||||
const ok = await editScriptHeader(
|
||||
one,
|
||||
(c) => scheduleInsteadOfAsset(c, asset, readsAsset(one, asset)),
|
||||
'Switched to a schedule'
|
||||
)
|
||||
if (!ok) return
|
||||
const saved = pe.setTriggerDraft({
|
||||
kind: 'schedule',
|
||||
config: { ...schedule, script_path: one }
|
||||
})
|
||||
if (!saved) {
|
||||
sendUserToast(
|
||||
`Another draft schedule already uses the path ${schedule.path}; ${one} has no schedule yet`,
|
||||
true
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
actions.push({
|
||||
label: one ? `Stop ${scriptName(one)} running on writes` : 'Stop them running on writes',
|
||||
detail: `${stopListeningChangeText(asset, reads)} in ${subscribers.join(', ')}; only the annotation header changes. Saved as a draft.`,
|
||||
run: () =>
|
||||
editScripts(
|
||||
subscribers,
|
||||
'Stopped running on writes',
|
||||
(s) => (c) => stopListeningToAsset(c, asset, readsAsset(s, asset))
|
||||
)
|
||||
})
|
||||
return actions
|
||||
}
|
||||
|
||||
let nodeFixes = $derived.by(() => {
|
||||
const fixes = new Map<string, NodeFixSpec>()
|
||||
if (isOperator) return fixes
|
||||
for (const { asset, subscribers } of nodeErrors.unwritten.values()) {
|
||||
fixes.set(`asset:${asset.kind}:${asset.path}`, {
|
||||
explainer: [
|
||||
...(subscribers.length === 1
|
||||
? [scriptLink(subscribers[0]), ' runs']
|
||||
: [`${subscribers.length} scripts run`]),
|
||||
' after each write to ',
|
||||
assetLink(asset),
|
||||
`, but nothing writes it, so ${subscribers.length === 1 ? 'it never runs' : 'they never run'}.`
|
||||
],
|
||||
actions: subscriptionFixes(asset, subscribers)
|
||||
})
|
||||
}
|
||||
for (const [path, asset] of nodeErrors.waitsOn) {
|
||||
fixes.set(`script:${path}`, {
|
||||
explainer: [
|
||||
@@ -1464,7 +1393,16 @@
|
||||
', but nothing writes it, so it never runs.'
|
||||
],
|
||||
actions: [
|
||||
...subscriptionFixes(asset, [path]),
|
||||
{
|
||||
label: `Remove \`on ${assetRef(asset)}\``,
|
||||
detail: `${stopListeningChangeText(asset, readsAsset(path, asset))} in ${path}; only the annotation header changes. Saved as a draft.`,
|
||||
run: () =>
|
||||
editScriptHeader(
|
||||
path,
|
||||
(c) => stopListeningToAsset(c, asset, readsAsset(path, asset)),
|
||||
`Removed \`on ${assetRef(asset)}\``
|
||||
)
|
||||
},
|
||||
{
|
||||
label: 'Open script',
|
||||
run: () => {
|
||||
|
||||
Reference in New Issue
Block a user