mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-16 00:03:02 +00:00
fix: a page keeps its path segment when it cannot offer a rename
Four things the local reviews caught in the rename work: A detail page handed the band a `pathTrigger` whether or not it could save one. Without `onSaved` — every script, flow or agent page the reader cannot write — `SummaryPathDisplay` fell through to its read-only branch, which ignores the `label` it was given and draws its own block: the trail lost its path and the summary appeared twice, two lines deep, in a 44px bar. The page now offers the trigger only where a rename can be saved, and the component renders what it was handed rather than its own row. The band gives up its own segment the moment a page offers a `pathTrigger`, so a trigger whose own gate is closed left the trail with no path at all — for the whole of an agent draft's load, not just a flush. Each gated snippet now draws the path itself in that case. `openFocus` named the path field as `#path`, which melt resolves with a document-wide query; `Path` hardcodes that id at every call site, and the popover is portalled to the body, so it lost document order to any other `Path` on the page. It is scoped to the popover's own field now. And `open` was an exported `$bindable(false)` on an optional prop, which the frontend's own rules ban. No caller bound it, so it is local state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Guilhem Lemouel
co-authored by
Claude Opus 5
Claude Opus 5
parent
1bc53e3877
commit
d0e14b05e1
@@ -1667,8 +1667,9 @@
|
||||
path, with the cursor in the path field. `bind:` cannot be spread, so the slots are
|
||||
written out twice; both instances bind the same ones and only one is ever open. -->
|
||||
{#snippet flowPathTrigger(pathLabel: Snippet, triggerClass: string)}
|
||||
<!-- Gated the way the script editor gates its own: this renders from the band's tree,
|
||||
which outlives what it reads here by a flush. -->
|
||||
<!-- Gated the way the script editor gates its own, `{:else}` included: this renders from
|
||||
the band's tree, which outlives what it reads here by a flush, and the band has
|
||||
already given up its own segment by the time this decides. -->
|
||||
{#if flowStore?.val}
|
||||
<PathEditPopover
|
||||
label={pathLabel}
|
||||
@@ -1684,6 +1685,8 @@
|
||||
onBehalfOfEmail={$savedOnBehalfOfEmail}
|
||||
workspaceId={autosaveWorkspace}
|
||||
/>
|
||||
{:else}
|
||||
<span class={triggerClass}>{@render pathLabel()}</span>
|
||||
{/if}
|
||||
{/snippet}
|
||||
|
||||
|
||||
@@ -92,8 +92,8 @@ The row's height matches the sidebar's own header row, so the two read as one ba
|
||||
// wants: the trail truncating from its start.
|
||||
phone ? 'h-11 pl-1 pr-2' : 'h-11 pl-2 pr-4',
|
||||
// The filling surface is what meets the right edge, so the bar keeps no gutter of its own
|
||||
// there — and it cannot wrap, since that surface is as tall as the bar.
|
||||
content?.actionsFill && 'pr-0 flex-nowrap',
|
||||
// there.
|
||||
content?.actionsFill && 'pr-0',
|
||||
// The small left padding is there to sit a button against: the sidebar handle, or the
|
||||
// workspace disc, both of which carry their own visual margin. Embedded there is neither,
|
||||
// and the page's name would start as bare text 8px from the edge.
|
||||
@@ -132,14 +132,14 @@ The row's height matches the sidebar's own header row, so the two read as one ba
|
||||
workspace and the folders before it ever reaches the name of the thing the page is about.
|
||||
The 5rem floor is what is left when every name in it has gone — the workspace disc, the
|
||||
fork mark and the environment badge, which is as small as the part can be and still say
|
||||
where the user is. Only below that does the bar wrap and give the trail a line of its own.
|
||||
where the user is.
|
||||
An embed (`navHidden`) keeps the page's own name but drops the workspace part around it:
|
||||
the trail would offer to navigate the host's workspace, while the name is what says which
|
||||
page the controls beside it belong to. -->
|
||||
<div class="flex shrink-[0.3] min-w-[5rem]">
|
||||
<!-- Bridged like the actions below: what a page hangs off its own name renders here, out
|
||||
of the tree that named it, and the pen in there asks that tree who the acting user is
|
||||
before it offers to rename anything. -->
|
||||
<!-- Bridged like the actions below: what a page hangs off its own name renders here, out of
|
||||
the tree that named it, and the rename editor in there asks that tree who the acting user
|
||||
is before it offers to rename anything. -->
|
||||
<!-- Each fragment the band renders comes from another tree and reads that tree's state. The
|
||||
registry already tolerates a registration that cannot answer while its owner's data is
|
||||
being cleared (see `read`); the same window reaches rendering, and an error here would
|
||||
@@ -165,9 +165,8 @@ The row's height matches the sidebar's own header row, so the two read as one ba
|
||||
<!-- No kind icon: the page below is the item, and saying "this is a flow" above a flow
|
||||
editor tells the reader what they can already see. -->
|
||||
<div class={twMerge('flex items-center gap-1 min-w-0', phone && 'shrink')}>
|
||||
<!-- A dot rather than a slash: the summary names the same item the path just located,
|
||||
it is not another level of it. Inside this box so that a bar wrapping on a phone
|
||||
carries it down with the summary instead of stranding it on the line above. -->
|
||||
<!-- A dot rather than a slash: the summary names the same item the path just located, it
|
||||
is not another level of it. Inside this box so that it narrows with the summary. -->
|
||||
<span class="shrink-0 text-hint/40 text-xs px-0.5" aria-hidden="true">·</span>
|
||||
{#if item.summaryContent}
|
||||
<svelte:boundary onerror={(e) => console.error('page header: summary failed to render', e)}>
|
||||
|
||||
@@ -59,8 +59,6 @@ follow.
|
||||
* click opens an editor here rather than going anywhere, and the fill is what says the
|
||||
* name is a control at all now that no pen sits beside it. */
|
||||
triggerClass?: string
|
||||
/** Bindable, for a host that drives the popover from elsewhere. */
|
||||
open?: boolean
|
||||
/** Which field takes the cursor on open. A popover anchored under the path opens on the
|
||||
* path, one under the summary opens on the summary: the click says which half of the name
|
||||
* the user came to change. `Path` autofocuses itself, so 'path' is the absence of a
|
||||
@@ -84,7 +82,6 @@ follow.
|
||||
penVisibility = 'hover',
|
||||
label,
|
||||
triggerClass = 'inline-flex items-center gap-1.5 min-w-0 max-w-full px-1 py-0.5 rounded cursor-pointer text-left hover:bg-surface-hover transition-colors',
|
||||
open = $bindable(false),
|
||||
focusField = 'summary',
|
||||
snapshotPath = $bindable(),
|
||||
error = $bindable()
|
||||
@@ -98,10 +95,20 @@ follow.
|
||||
: 'Edit path'
|
||||
)
|
||||
|
||||
// The popover's own state: a host opens this editor by clicking the name it handed over, so
|
||||
// there is nothing for one to drive from outside.
|
||||
let open = $state(false)
|
||||
|
||||
// Treat an empty path as ownable so the popover lets a user pick the path for a brand-new
|
||||
// item. `Path.reset()` then synthesizes a default under their own user/folder scope.
|
||||
const own = $derived(pathEditable && (!path || isOwner(path, actingUser, $operatingWorkspace)))
|
||||
|
||||
/** The path's name field inside this popover. `Path` hardcodes `id="path"`, and melt resolves a
|
||||
* string `openFocus` with a document-wide `querySelector`, so a popover portalled to the body
|
||||
* would hand the keystrokes to whichever other `Path` the page happens to have mounted. */
|
||||
const pathField = () =>
|
||||
document.querySelector<HTMLInputElement>('[data-path-edit-path] #path') ?? null
|
||||
|
||||
function setOpen(v: boolean) {
|
||||
open = v
|
||||
// Only snapshot when there is a path to freeze. A new flow or script opens with `path === ''`
|
||||
@@ -111,8 +118,8 @@ follow.
|
||||
}
|
||||
</script>
|
||||
|
||||
<!-- `openFocus`: `#path` is the name field inside `Path`. Without naming it melt lands on the
|
||||
row's first button, so a popover opened from the path would not be typing into the path. -->
|
||||
<!-- `openFocus`: without naming the path's field melt lands on the row's first button, so a
|
||||
popover opened from the path would not be typing into the path. -->
|
||||
<Popover
|
||||
placement="bottom-start"
|
||||
contentClasses="p-4"
|
||||
@@ -123,7 +130,7 @@ follow.
|
||||
disableFocusTrap
|
||||
closeOnOtherPopoverOpen
|
||||
openFocus={focusField === 'path'
|
||||
? '#path'
|
||||
? pathField
|
||||
: summaryEditable
|
||||
? '[data-path-edit-summary]'
|
||||
: undefined}
|
||||
@@ -178,19 +185,21 @@ follow.
|
||||
would make a rename to a taken path look untaken the moment the popover is
|
||||
reopened on it. With no saved path there is nothing the item occupies, so every
|
||||
path is checked. -->
|
||||
<Path
|
||||
autofocus
|
||||
bind:path
|
||||
bind:error
|
||||
initialPath={savedPath ?? path ?? ''}
|
||||
allowedExistingPath={savedPath}
|
||||
checkInitialPathExistence={savedPath == undefined}
|
||||
namePlaceholder={kind}
|
||||
{kind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
workspaceOverride={workspaceId}
|
||||
/>
|
||||
<div data-path-edit-path>
|
||||
<Path
|
||||
autofocus
|
||||
bind:path
|
||||
bind:error
|
||||
initialPath={savedPath ?? path ?? ''}
|
||||
allowedExistingPath={savedPath}
|
||||
checkInitialPathExistence={savedPath == undefined}
|
||||
namePlaceholder={kind}
|
||||
{kind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
workspaceOverride={workspaceId}
|
||||
/>
|
||||
</div>
|
||||
{#if savedPath && path && path !== savedPath}
|
||||
<Alert
|
||||
type="info"
|
||||
|
||||
@@ -2392,9 +2392,9 @@
|
||||
both instances bind the same slots, and only one is ever open. -->
|
||||
{#snippet scriptPathTrigger(pathLabel: Snippet, triggerClass: string)}
|
||||
<!-- Gated like the summary beside it: `script` is a bindable prop the route can clear in
|
||||
the same flush that drops this editor, and this renders from the band's tree. Without
|
||||
the trigger the band draws its own path segment, which is what it does for every page
|
||||
that offers no rename. -->
|
||||
the same flush that drops this editor, and this renders from the band's tree. The
|
||||
`{:else}` is not decoration: the band hands the segment over the moment a page offers
|
||||
a `pathTrigger`, so a gate that drew nothing would take the path off the trail. -->
|
||||
{#if script}
|
||||
<PathEditPopover
|
||||
label={pathLabel}
|
||||
@@ -2409,6 +2409,8 @@
|
||||
kind="script"
|
||||
workspaceId={autosaveWorkspace}
|
||||
/>
|
||||
{:else}
|
||||
<span class={triggerClass}>{@render pathLabel()}</span>
|
||||
{/if}
|
||||
{/snippet}
|
||||
|
||||
|
||||
@@ -87,6 +87,12 @@
|
||||
}
|
||||
})
|
||||
|
||||
/** The path's name field inside this popover. `Path` hardcodes `id="path"`, and melt resolves a
|
||||
* string `openFocus` with a document-wide `querySelector`, so a popover portalled to the body
|
||||
* would hand the keystrokes to whichever other `Path` the page happens to have mounted. */
|
||||
const pathField = () =>
|
||||
document.querySelector<HTMLInputElement>('[data-path-edit-path] #path') ?? null
|
||||
|
||||
async function save(close: () => void) {
|
||||
const initialPath = path ?? ''
|
||||
const newPath = own ? editPath : initialPath
|
||||
@@ -111,8 +117,8 @@
|
||||
</script>
|
||||
|
||||
{#snippet editor()}
|
||||
<!-- `#path` is the name field inside `Path`; without naming it melt lands on the row's first
|
||||
button, so a popover opened from the path would not be typing into the path. -->
|
||||
<!-- Without naming the path's own field melt lands on the row's first button, so a popover
|
||||
opened from the path would not be typing into the path. -->
|
||||
<Popover
|
||||
class={triggerClass}
|
||||
placement="bottom-start"
|
||||
@@ -122,7 +128,7 @@
|
||||
excludeSelectors=".drawer"
|
||||
disableFocusTrap
|
||||
openFocus={focusField === 'path' && own
|
||||
? '#path'
|
||||
? pathField
|
||||
: () => {
|
||||
summaryInput?.focus()
|
||||
return null
|
||||
@@ -182,16 +188,18 @@
|
||||
{/if}
|
||||
<Label label="Path">
|
||||
{#if own}
|
||||
<Path
|
||||
autofocus={false}
|
||||
bind:path={editPath}
|
||||
bind:dirty={dirtyPath}
|
||||
initialPath={path ?? ''}
|
||||
namePlaceholder={kind}
|
||||
kind={pathKind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
/>
|
||||
<div data-path-edit-path>
|
||||
<Path
|
||||
autofocus={false}
|
||||
bind:path={editPath}
|
||||
bind:dirty={dirtyPath}
|
||||
initialPath={path ?? ''}
|
||||
namePlaceholder={kind}
|
||||
kind={pathKind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
/>
|
||||
</div>
|
||||
{:else}
|
||||
<span class="text-xs font-mono text-secondary">{path}</span>
|
||||
<p class="text-2xs text-tertiary mt-1">Only the owner can change the path</p>
|
||||
@@ -230,16 +238,18 @@
|
||||
</label>
|
||||
<div class="block text-primary">
|
||||
<div class="pb-1 text-xs font-semibold text-emphasis">Path</div>
|
||||
<Path
|
||||
autofocus={false}
|
||||
bind:path
|
||||
bind:dirty={dirtyPath}
|
||||
initialPath={path ?? ''}
|
||||
namePlaceholder={kind}
|
||||
kind={pathKind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
/>
|
||||
<div data-path-edit-path>
|
||||
<Path
|
||||
autofocus={false}
|
||||
bind:path
|
||||
bind:dirty={dirtyPath}
|
||||
initialPath={path ?? ''}
|
||||
namePlaceholder={kind}
|
||||
kind={pathKind}
|
||||
size="sm"
|
||||
drawerOffset={4000}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
{/if}
|
||||
</div>
|
||||
@@ -279,6 +289,10 @@
|
||||
<InheritedLabels labels={inheritedLabels} />
|
||||
</div>
|
||||
</div>
|
||||
{:else if label}
|
||||
<!-- Nothing to open, but the host still drew this into a slot of its own: give back what it
|
||||
handed over rather than this component's own block, which belongs in a row it owns. -->
|
||||
{@render label()}
|
||||
{:else}
|
||||
<div class="min-w-0 truncate flex items-center {compact ? '' : 'flex-col px-2'}">
|
||||
{#if !emptyString(summary) && !compact}
|
||||
|
||||
@@ -269,7 +269,14 @@
|
||||
<!-- The summary keeps its rename-and-labels popover here rather than becoming plain text in
|
||||
the breadcrumb: renaming a script or flow is done from this page, not from the trail. -->
|
||||
<PageHeaderContent
|
||||
item={{ kind: errorHandlerKind, path, summaryContent, pathTrigger }}
|
||||
item={{
|
||||
kind: errorHandlerKind,
|
||||
path,
|
||||
summaryContent,
|
||||
// Only where this page can save one: without `onSaved` the editor behind it has nothing
|
||||
// to write, and the band keeps its own segment, which copies the path.
|
||||
pathTrigger: onSaved ? pathTrigger : undefined
|
||||
}}
|
||||
{actions}
|
||||
contexts={headerContexts}
|
||||
separator="always"
|
||||
|
||||
@@ -339,8 +339,9 @@
|
||||
{#snippet agentPathTrigger(pathLabel: Snippet, triggerClass: string)}
|
||||
<!-- Gated like the summary beside it: this renders from the band's tree, where `draft` is
|
||||
undefined for as long as the modal has not loaded one, and a getter that reads through it
|
||||
throws there rather than where it was written. Without the trigger the band draws its own
|
||||
path segment, which is what it does for every page that offers no rename. -->
|
||||
throws there rather than where it was written. The `{:else}` carries the whole load of
|
||||
that wait: the band gave up its own segment to this snippet, so without it the trail
|
||||
would show no path until the draft arrives. -->
|
||||
{#if draft?.state}
|
||||
<PathEditPopover
|
||||
label={pathLabel}
|
||||
@@ -366,6 +367,8 @@
|
||||
pathEditable={!readOnly}
|
||||
summaryEditable={!readOnly}
|
||||
/>
|
||||
{:else}
|
||||
<span class={triggerClass}>{@render pathLabel()}</span>
|
||||
{/if}
|
||||
{/snippet}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user