improvement(forking): minor ui improvements (#5533)

* improvement(forking): minor ui improvements

* fix warning
This commit is contained in:
Vikhyath Mondreti
2026-07-08 22:53:52 -07:00
committed by GitHub
parent 472457b082
commit fea3391015
14 changed files with 248 additions and 133 deletions
@@ -16,11 +16,11 @@ interface SettingsUnsavedGuard {
/**
* Wires a settings surface's local dirty state into the shared
* `useSettingsDirtyStore`, so the sidebar's section-switch confirmation and the
* centralized `beforeunload` both apply without per-page wiring. Also provides
* an in-view back/close guard (`guardBack` + the shared `UnsavedChangesModal`)
* for detail sub-views whose "back" is an in-component state change rather than
* a route navigation.
* `useSettingsDirtyStore`, so the sidebar's leave confirmation (section switch,
* Back, workspace switch) and the centralized `beforeunload` both apply without
* per-page wiring. Also provides an in-view back/close guard (`guardBack` + the
* shared `UnsavedChangesModal`) for detail sub-views whose "back" is an
* in-component state change rather than a route navigation.
*/
export function useSettingsUnsavedGuard({
isDirty,
@@ -51,12 +51,12 @@ export function SettingsSidebar({
const queryClient = useQueryClient()
const requestNavigation = useSettingsDirtyStore((s) => s.requestNavigation)
const confirmNavigation = useSettingsDirtyStore((s) => s.confirmNavigation)
const cancelNavigation = useSettingsDirtyStore((s) => s.cancelNavigation)
const isDirty = useSettingsDirtyStore((s) => s.isDirty)
const requestLeave = useSettingsDirtyStore((s) => s.requestLeave)
const confirmLeave = useSettingsDirtyStore((s) => s.confirmLeave)
const cancelLeave = useSettingsDirtyStore((s) => s.cancelLeave)
const pendingLeave = useSettingsDirtyStore((s) => s.pendingLeave)
const showDiscardDialog = pendingLeave !== null
const [showDiscardDialog, setShowDiscardDialog] = useState(false)
const [hasOverflowTop, setHasOverflowTop] = useState(false)
const { data: session } = useSession()
@@ -217,27 +217,18 @@ export function SettingsSidebar({
const { popSettingsReturnUrl, getSettingsHref } = useSettingsNavigation()
const handleBack = useCallback(() => {
if (isDirty) {
setShowDiscardDialog(true)
return
}
router.push(popSettingsReturnUrl(`/workspace/${workspaceId}/home`))
}, [router, popSettingsReturnUrl, workspaceId, isDirty])
requestLeave(() => {
router.push(popSettingsReturnUrl(`/workspace/${workspaceId}/home`))
})
}, [requestLeave, router, popSettingsReturnUrl, workspaceId])
const handleConfirmDiscard = useCallback(() => {
const section = confirmNavigation()
setShowDiscardDialog(false)
if (section) {
router.replace(getSettingsHref({ section }), { scroll: false })
} else {
router.push(popSettingsReturnUrl(`/workspace/${workspaceId}/home`))
}
}, [confirmNavigation, router, getSettingsHref, popSettingsReturnUrl, workspaceId])
confirmLeave()
}, [confirmLeave])
const handleCancelDiscard = useCallback(() => {
cancelNavigation()
setShowDiscardDialog(false)
}, [cancelNavigation])
cancelLeave()
}, [cancelLeave])
useEffect(() => {
const container = scrollContainerRef.current
@@ -350,11 +341,9 @@ export function SettingsSidebar({
onClick={() => {
const section = item.id as SettingsSection
if (section === activeSection) return
if (!requestNavigation(section)) {
setShowDiscardDialog(true)
return
}
router.replace(getSettingsHref({ section }), { scroll: false })
requestLeave(() => {
router.replace(getSettingsHref({ section }), { scroll: false })
})
}}
>
{content}
@@ -110,6 +110,7 @@ import { SIDEBAR_WIDTH } from '@/stores/constants'
import { useFolderStore } from '@/stores/folders/store'
import { useSearchModalStore } from '@/stores/modals/search/store'
import { useProvidersStore } from '@/stores/providers'
import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'
import { useSidebarStore } from '@/stores/sidebar/store'
const logger = createLogger('Sidebar')
@@ -1072,16 +1073,21 @@ export const Sidebar = memo(function Sidebar({ isCollapsed }: SidebarProps) {
fileInputRef.current?.click()
}
const requestLeave = useSettingsDirtyStore((s) => s.requestLeave)
const handleWorkspaceSwitch = useCallback(
async (workspace: Workspace) => {
(workspace: Workspace) => {
if (workspace.id === workspaceId) {
setIsWorkspaceMenuOpen(false)
return
}
await switchWorkspace(workspace)
// Close the switcher first so the settings discard dialog (if any) is visible.
setIsWorkspaceMenuOpen(false)
requestLeave(() => {
void switchWorkspace(workspace)
})
},
[workspaceId, switchWorkspace]
[workspaceId, switchWorkspace, requestLeave]
)
const handleSidebarClick = (e: React.MouseEvent<HTMLElement>) => {
@@ -30,6 +30,9 @@ interface ResourceKindRowProps {
* the user can copy a specific subset. Shared by the fork modal's "Copy resources" and the sync
* modal's "Copy resources" so the two surfaces stay identical. Files nest in a folder tree
* instead - use {@link FileKindRow}.
*
* Expanded body uses the settings MCP expand pattern (`border-t` + `--surface-2`) so items
* read as contained in the kind rather than indented siblings of other kinds.
*/
export function ResourceKindRow({
label,
@@ -47,7 +50,7 @@ export function ResourceKindRow({
const headerState = selectedCount === 0 ? false : selectedCount === total ? true : 'indeterminate'
return (
<div className='flex flex-col gap-1'>
<div className='flex flex-col'>
<div className='flex items-center gap-2 text-[var(--text-body)] text-sm'>
<Checkbox
size='sm'
@@ -79,32 +82,34 @@ export function ResourceKindRow({
</div>
{expanded ? (
<div className='ml-6 flex max-h-44 flex-col gap-0.5 overflow-y-auto'>
{items.map((item) => {
const isChecked = selected.has(item.id)
const itemId = `${fieldId}-${item.id}`
return (
<label
key={item.id}
htmlFor={itemId}
className={cn(
'flex min-w-0 items-center gap-2 rounded-md py-0.5 text-[var(--text-body)] text-sm',
disabled
? 'cursor-not-allowed opacity-60'
: 'cursor-pointer hover:text-[var(--text-primary)]'
)}
>
<Checkbox
id={itemId}
size='sm'
checked={isChecked}
onCheckedChange={(checked) => onToggleItem(item.id, checked === true)}
disabled={disabled}
/>
<span className='truncate'>{item.label}</span>
</label>
)
})}
<div className='mt-1 max-h-44 overflow-y-auto border-[var(--border-1)] border-t bg-[var(--surface-2)] px-2.5 py-2'>
<div className='flex flex-col gap-0.5'>
{items.map((item) => {
const isChecked = selected.has(item.id)
const itemId = `${fieldId}-${item.id}`
return (
<label
key={item.id}
htmlFor={itemId}
className={cn(
'flex min-w-0 items-center gap-2 rounded-md py-0.5 text-[var(--text-body)] text-sm',
disabled
? 'cursor-not-allowed opacity-60'
: 'cursor-pointer hover:text-[var(--text-primary)]'
)}
>
<Checkbox
id={itemId}
size='sm'
checked={isChecked}
onCheckedChange={(checked) => onToggleItem(item.id, checked === true)}
disabled={disabled}
/>
<span className='truncate'>{item.label}</span>
</label>
)
})}
</div>
</div>
) : null}
</div>
@@ -145,7 +150,7 @@ export function FileKindRow({
const { folders, rootFiles } = useMemo(() => groupForkFilesIntoFolders(files), [files])
return (
<div className='flex flex-col gap-1'>
<div className='flex flex-col'>
<div className='flex items-center gap-2 text-[var(--text-body)] text-sm'>
<Checkbox
size='sm'
@@ -172,7 +177,7 @@ export function FileKindRow({
</div>
{expanded ? (
<div className='ml-6'>
<div className='mt-1 border-[var(--border-1)] border-t bg-[var(--surface-2)] px-2.5 py-2'>
<ForkFileTree
folders={folders}
rootFiles={rootFiles}
@@ -228,8 +228,8 @@ interface DependentWorkflowCardProps {
/**
* One workflow's dependent fields as a collapsible card (the same `CollapsibleCard` the table
* workflow sidebar's input mapping and the enrichment config use): the header names the
* workflow, the body groups fields under their block's label. Cards holding a required field
* start expanded - a required field is what gates Sync.
* workflow; the body groups fields under block → optional tool → plain field label.
* Cards holding a required field start expanded - a required field is what gates Sync.
*/
function DependentWorkflowCard({
workflow,
@@ -250,30 +250,70 @@ function DependentWorkflowCard({
collapsed={collapsed}
onToggleCollapse={() => setCollapsed((value) => !value)}
>
{workflow.blocks.map((block) => (
<div key={block.targetBlockId} className='flex flex-col gap-1.5'>
<Label className='text-small'>{block.blockName}</Label>
{block.fields.map((field) => (
<div key={dependentKey(field)} className='flex flex-col gap-1'>
<span className='text-[var(--text-muted)] text-caption'>
{field.title}
{field.required ? <span className='text-[var(--text-error)]'> *</span> : null}
</span>
<DependentSelector
field={field}
block={block}
target={target}
parentChanged={parentChanged}
copying={copying}
workspaceId={workspaceId}
sourceWorkspaceId={sourceWorkspaceId}
reconfig={reconfig}
setReconfig={setReconfig}
/>
<div className='flex flex-col gap-3'>
{workflow.blocks.map((block) => {
const topLevel = block.fields.filter((field) => !field.toolName)
const byTool = new Map<string, ForkDependentReconfig[]>()
for (const field of block.fields) {
if (!field.toolName) continue
const list = byTool.get(field.toolName)
if (list) list.push(field)
else byTool.set(field.toolName, [field])
}
const toolGroups = Array.from(byTool.entries()).sort(([a], [b]) => a.localeCompare(b))
return (
<div key={block.targetBlockId} className='flex flex-col gap-2'>
<Label className='text-small'>{block.blockName}</Label>
{topLevel.map((field) => (
<div key={dependentKey(field)} className='flex flex-col gap-1'>
<Label className='text-[var(--text-muted)] text-caption'>
{field.title}
{field.required ? <span className='text-[var(--text-error)]'> *</span> : null}
</Label>
<DependentSelector
field={field}
block={block}
target={target}
parentChanged={parentChanged}
copying={copying}
workspaceId={workspaceId}
sourceWorkspaceId={sourceWorkspaceId}
reconfig={reconfig}
setReconfig={setReconfig}
/>
</div>
))}
{toolGroups.map(([toolName, fields]) => (
<div key={toolName} className='flex flex-col gap-1.5 pl-2'>
<span className='text-[var(--text-muted)] text-small'>{toolName}</span>
{fields.map((field) => (
<div key={dependentKey(field)} className='flex flex-col gap-1'>
<Label className='text-[var(--text-muted)] text-caption'>
{field.title}
{field.required ? (
<span className='text-[var(--text-error)]'> *</span>
) : null}
</Label>
<DependentSelector
field={field}
block={block}
target={target}
parentChanged={parentChanged}
copying={copying}
workspaceId={workspaceId}
sourceWorkspaceId={sourceWorkspaceId}
reconfig={reconfig}
setReconfig={setReconfig}
/>
</div>
))}
</div>
))}
</div>
))}
</div>
))}
)
})}
</div>
</CollapsibleCard>
)
}
@@ -87,7 +87,7 @@ interface ForkSyncDetailViewProps {
otherWorkspaceId: string
otherWorkspaceName: string
onBack: () => void
/** Header chips rendered left of Sync (Rollback / Open workspace) — the caller owns those. */
/** Header chips rendered left of Sync (e.g. Open workspace) — the caller owns those. */
actions: SettingsAction[]
}
@@ -129,8 +129,8 @@ function ForkSyncDetailView({
const [confirmSyncOpen, setConfirmSyncOpen] = useState(false)
// Sync is the edge's primary action, so it's the rightmost/black chip; the caller's
// Rollback/Open workspace chips sit left of it. Dirty mapping edits swap the whole
// cluster for Discard/Save until they're saved or discarded.
// Open workspace chip sits left of it. Dirty mapping edits swap the whole cluster
// for Discard/Save until they're saved or discarded.
const panelActions: SettingsAction[] = controller.dirty
? saveDiscardActions({
dirty: controller.dirty,
@@ -263,8 +263,9 @@ function ForkActivityDetailView({
* "Parent" section, above the "Forks" list of child forks. The parent row's `...` menu
* has Edit mappings (the child owns its edge's re-picks), Open workspace, and
* Disconnect; fork rows offer Open workspace and Disconnect only. Activity is
* workspace-scoped and lives behind the header's "See activity" action; sync/rollback
* live on the parent's sync detail page.
* workspace-scoped and lives behind the header's "See activity" action (including
* Rollback when the last sync into this workspace is undoable). Sync lives on the
* parent's sync detail page.
* Forking and sync rewrite workflow state and deployments en masse, so the page is
* workspace-admin only and gated on the workspace's fork entitlement - every fork route
* re-checks both; the server remains the boundary.
@@ -374,21 +375,10 @@ export function Forks() {
// deep link falls back to the list). Fork rows offer Open workspace / Disconnect only.
const showParentDetail = Boolean(selectedForkId && parent && parent.id === selectedForkId)
// Rollback (destructive) and Open workspace sit left of the detail view's primary Sync
// chip, which the sync page owns (it carries the gating).
// Open workspace sits left of the detail view's primary Sync chip, which the sync
// page owns (it carries the gating). Rollback lives on the Activity view only.
const parentHeaderActions: SettingsAction[] = parent
? [
...(undoableRun
? [
{
text: 'Rollback',
variant: 'destructive' as const,
onSelect: () => setConfirmRollbackOpen(true),
disabled: rollback.isPending,
tooltip: `The last sync into this workspace (from ${undoableRun.otherName}) can be undone — it restores each workflow's prior deployed version.`,
},
]
: []),
{
text: 'Open workspace',
onSelect: () => openForkWorkspace(parent.id),
@@ -369,7 +369,8 @@ describe('collectForkDependentReconfigs', () => {
blockName: 'Block',
subBlockKey: 'tools[0].folder',
selectorKey: 'gmail.labels',
title: 'Gmail 1: Label',
title: 'Label',
toolName: 'Gmail 1',
currentValue: 'INBOX',
required: true,
consumesContextKeys: [],
@@ -494,7 +495,11 @@ describe('collectForkDependentReconfigs', () => {
])
const result = collectForkDependentReconfigs([replaceItem], states, resolve)
expect(result).toHaveLength(1)
expect(result[0]).toMatchObject({ subBlockKey: 'tools[0].folder', title: 'Gmail 1: Label' })
expect(result[0]).toMatchObject({
subBlockKey: 'tools[0].folder',
title: 'Label',
toolName: 'Gmail 1',
})
})
it('evaluates a nested tool selector condition against the tool-level operation', () => {
@@ -74,6 +74,8 @@ interface EmitAnchoredParams {
/** Map a dependent's config id to its wire `subBlockKey` (identity, or nested `tools[i].id`). */
makeSubBlockKey: (dependentId: string) => string
makeTitle: (dependent: SubBlockConfig) => string
/** Nested `tool-input` tool display name; omitted for top-level block subblocks. */
toolName?: string
/**
* Emit `providesContextKey`/`consumesContextKeys` so the modal can chain in-block
* re-picks. Top-level chains; nested tool params don't (a tool's chain would need
@@ -102,6 +104,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void {
resolveTargetBlockId,
makeSubBlockKey,
makeTitle,
toolName,
chaining,
out,
} = params
@@ -202,6 +205,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void {
subBlockKey: makeSubBlockKey(dependent.id),
selectorKey: dependent.selectorKey,
title: makeTitle(dependent),
...(toolName ? { toolName } : {}),
// Source value, so the always-on listing pre-fills a stable parent's selector.
// The diff route overlays the stored/target-draft value onto `currentValue`;
// `sourceValue` stays the raw source reference (the copy-resolved parent's seed).
@@ -281,8 +285,8 @@ export function collectForkDependentReconfigs(
})
// Nested `tool-input` tools: each selected tool's own credential-anchored selectors,
// keyed `toolInput[index].paramId` (matching the needs-config key) and titled with the
// tool so the modal re-picks them under the same block card.
// keyed `toolInput[index].paramId` (matching the needs-config key). Field `title` stays
// plain; `toolName` carries the tool so the UI can show block → tool → field tiers.
for (const cfg of config.subBlocks) {
if (cfg.type !== 'tool-input' || !cfg.id) continue
const { array: tools } = coerceObjectArray(subBlocks[cfg.id]?.value)
@@ -318,7 +322,8 @@ export function collectForkDependentReconfigs(
canonicalModes: scopeCanonicalModesForTool(block.data?.canonicalModes, tool.type),
resolveTargetBlockId: resolveBlockId,
makeSubBlockKey: (id) => `${toolInputKey}[${toolIndex}].${id}`,
makeTitle: (dependent) => `${toolLabel}: ${dependent.title ?? dependent.id ?? ''}`,
makeTitle: (dependent) => dependent.title ?? dependent.id ?? '',
toolName: toolLabel,
chaining: false,
out,
})
@@ -667,6 +667,63 @@ describe('collectForkClearedRefCandidates', () => {
])
})
it('prefixes nested toolName onto dependent fieldLabel so two tools with Label stay distinct', () => {
vi.mocked(getBlock).mockImplementation((type) => {
if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }])
if (type === 'gmail')
return blockWith([
{ id: 'credential', title: 'Credential', type: 'oauth-input' },
{
id: 'folder',
title: 'Label',
type: 'folder-selector',
dependsOn: ['credential'],
selectorKey: 'gmail.labels',
},
])
return undefined as unknown as BlockConfig
})
const result = collectForkClearedRefCandidates(
params({
items: [
{
sourceWorkflowId: 'wf-src',
targetWorkflowId: 'wf-tgt',
mode: 'create',
sourceMeta: { name: 'New Workflow' },
},
],
sourceStates: new Map([
[
'wf-src',
stateWith('agent', 'Agent', {
tools: {
type: 'tool-input',
value: [
{
type: 'gmail',
title: 'Gmail 1',
params: { credential: 'cred-src', folder: 'INBOX' },
},
{
type: 'gmail',
title: 'Gmail 2',
params: { credential: 'cred-src', folder: 'SENT' },
},
],
},
}),
],
]),
sourceLabels: new Map([['credential:cred-src', 'Work Gmail']]),
})
)
expect(result.map((ref) => ref.fieldLabel).sort()).toEqual(['Gmail 1: Label', 'Gmail 2: Label'])
expect(result.every((ref) => ref.cause === 'dependent' && ref.blockLabel === 'Agent')).toBe(
true
)
})
it('carries the knowledge-base parent on a document-selector dependent (so it can drop off)', () => {
vi.mocked(getBlock).mockReturnValue(
blockWith([
@@ -253,7 +253,11 @@ export function collectForkClearedRefCandidates(
workflowName: workflowNameByTarget.get(dependent.targetWorkflowId) ?? '',
blockId: dependent.targetBlockId,
blockLabel: dependent.blockName,
fieldLabel: dependent.title,
// Nested tool fields keep a plain `title` for the reconfigure UI; warnings need the
// tool disambiguator so two tools with the same field name stay distinguishable.
fieldLabel: dependent.toolName
? `${dependent.toolName}: ${dependent.title}`
: dependent.title,
kind: dependent.parentKind,
sourceId: dependent.parentSourceId,
sourceLabel: labelFor(dependent.parentKind, dependent.parentSourceId),
@@ -961,7 +961,8 @@ describe('collectClearedDependents', () => {
blockId: 'b1',
blockName: 'Agent',
subBlockKey: 'tools[0].folder',
title: 'Gmail: Label',
title: 'Label',
toolName: 'Gmail',
required: true,
},
])
@@ -1098,7 +1098,10 @@ export interface NeedsConfigurationField {
blockId: string
blockName: string
subBlockKey: string
/** Plain field title (e.g. `Label`), never a `Tool: Field` composite. */
title: string
/** Nested `tool-input` tool display name when the field lives under a tool. */
toolName?: string
required: boolean
}
@@ -1169,7 +1172,8 @@ function collectClearedToolParamDependents(
blockId,
blockName,
subBlockKey: `${toolInputKey}[${index}].${cfg.id}`,
title: `${toolLabel}: ${cfg.title ?? cfg.id}`,
title: cfg.title ?? cfg.id,
toolName: toolLabel,
required: isSubBlockRequired(cfg.required, mergedValues),
})
}
@@ -331,7 +331,13 @@ export const forkDependentReconfigSchema = z.object({
blockName: z.string(),
subBlockKey: z.string(),
selectorKey: z.string(),
/** Plain field title (e.g. `Label`), never a `Tool: Field` composite. */
title: z.string(),
/**
* Display name of the nested `tool-input` tool this field belongs to (e.g. `Gmail` /
* `Gmail 1`). Absent for top-level block subblocks.
*/
toolName: z.string().optional(),
/**
* The field's stored value (from the persisted mapping), so the always-on reconfigure listing
* pre-fills the selector with what the user last set. Empty string when unset; for an edge
+21 -18
View File
@@ -1,28 +1,28 @@
import { create } from 'zustand'
import { devtools } from 'zustand/middleware'
import type { SettingsSection } from '@/app/workspace/[workspaceId]/settings/navigation'
interface SettingsDirtyStore {
isDirty: boolean
pendingSection: SettingsSection | null
/** Leave action deferred until the user confirms discard. */
pendingLeave: (() => void) | null
setDirty: (dirty: boolean) => void
/**
* Call before navigating to a new section. Returns `true` if navigation may
* proceed immediately; returns `false` if there are unsaved changes — in that
* case `pendingSection` is set so a confirmation dialog can be shown.
* Call before leaving the current settings surface. If clean, runs `leave` immediately
* and returns `true`. If dirty, stashes `leave` and returns `false` so the shared
* discard dialog can confirm before running it.
*/
requestNavigation: (section: SettingsSection) => boolean
/** Clears dirty + pending state and returns the section to navigate to. */
confirmNavigation: () => SettingsSection | null
/** Cancels a pending navigation without clearing dirty state. */
cancelNavigation: () => void
requestLeave: (leave: () => void) => boolean
/** Clears dirty + pending state and runs the deferred leave action. */
confirmLeave: () => void
/** Cancels a pending leave without clearing dirty state. */
cancelLeave: () => void
/** Resets all state — call on component unmount. */
reset: () => void
}
const initialState = {
isDirty: false,
pendingSection: null as SettingsSection | null,
pendingLeave: null as (() => void) | null,
}
export const useSettingsDirtyStore = create<SettingsDirtyStore>()(
@@ -32,19 +32,22 @@ export const useSettingsDirtyStore = create<SettingsDirtyStore>()(
setDirty: (dirty) => set({ isDirty: dirty }),
requestNavigation: (section) => {
if (!get().isDirty) return true
set({ pendingSection: section })
requestLeave: (leave) => {
if (!get().isDirty) {
leave()
return true
}
set({ pendingLeave: leave })
return false
},
confirmNavigation: () => {
const { pendingSection } = get()
confirmLeave: () => {
const { pendingLeave } = get()
set({ ...initialState })
return pendingSection
pendingLeave?.()
},
cancelNavigation: () => set({ pendingSection: null }),
cancelLeave: () => set({ pendingLeave: null }),
reset: () => set({ ...initialState }),
}),