From 9c6b4dd3cd5ad7b273b4d734e018f321aaa9fb6d Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 17:25:12 -0700 Subject: [PATCH 01/17] feat(FN-6879): document workflow nodes in editor detail pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a per-node Help section to the workflow editor's node detail pane: description, configuration, inputs, outputs, and edges for every node kind. Keys off the effective kind (preserved IR kind) so graph-only policy nodes — auto-merge gate, branch-group member integration / promotion, PR and recovery nodes — get specific help instead of reading as a generic merge/gate/hold, and are flagged "Engine-managed". Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/workflow-node-help.md | 5 + .../app/components/WorkflowNodeEditor.css | 74 +++++ .../app/components/WorkflowNodeEditor.tsx | 37 ++- .../__tests__/WorkflowNodeEditor.test.tsx | 19 ++ .../nodes/__tests__/node-help.test.ts | 107 +++++++ .../app/components/nodes/node-help.ts | 294 ++++++++++++++++++ 6 files changed, 535 insertions(+), 1 deletion(-) create mode 100644 .changeset/workflow-node-help.md create mode 100644 packages/dashboard/app/components/nodes/__tests__/node-help.test.ts create mode 100644 packages/dashboard/app/components/nodes/node-help.ts diff --git a/.changeset/workflow-node-help.md b/.changeset/workflow-node-help.md new file mode 100644 index 0000000000..29a99275c9 --- /dev/null +++ b/.changeset/workflow-node-help.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": minor +--- + +Workflow editor: add a Help section to the node detail pane. Every node now documents what it does, how to configure it, and its inputs/outputs/edges — including the engine-managed merge-lifecycle nodes (auto-merge gate, branch-group member integration, branch-group promotion, PR and recovery nodes), which are surfaced read-only with an "Engine-managed" badge. diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.css b/packages/dashboard/app/components/WorkflowNodeEditor.css index e94ada1125..5c9c173cf2 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.css +++ b/packages/dashboard/app/components/WorkflowNodeEditor.css @@ -957,6 +957,80 @@ React Flow ships white default controls and mini-map chrome, but the workflow ed color: var(--ws-warning); } +/* ── Per-node Help (FNXC:WorkflowEditor 2026-06-21-10:00) ─────────── + * Collapsible
teaching what the selected node does, how to + * configure it, and its inputs/outputs/edges. Sits under the heading, + * collapsed by default so it never pushes config fields below the fold. */ +.wf-inspector-help { + border: 1px solid var(--border); + border-radius: var(--radius-sm); + background: var(--bg-secondary); +} + +.wf-inspector-help-summary { + display: flex; + align-items: center; + gap: var(--space-xs); + padding: var(--space-xs) var(--space-sm); + font-size: 0.78rem; + color: var(--text); + cursor: pointer; + list-style: none; + user-select: none; +} + +.wf-inspector-help-summary::-webkit-details-marker { + display: none; +} + +.wf-inspector-help-summary:hover { + background: var(--bg-tertiary); + border-radius: var(--radius-sm); +} + +/* Engine-managed badge for graph-only policy nodes (read-only lifecycle). */ +.wf-inspector-help-badge { + margin-left: auto; + padding: 1px var(--space-xs); + font-size: 0.66rem; + text-transform: uppercase; + letter-spacing: 0.03em; + color: var(--text-dim); + border: 1px solid var(--border); + border-radius: var(--radius-sm); +} + +.wf-inspector-help-body { + display: flex; + flex-direction: column; + gap: var(--space-xs); + padding: 0 var(--space-sm) var(--space-sm); + font-size: 0.76rem; + color: var(--text-muted); +} + +.wf-inspector-help-summary-text { + margin: 0; + color: var(--text); +} + +.wf-inspector-help-dl { + display: grid; + grid-template-columns: max-content 1fr; + gap: 2px var(--space-sm); + margin: 0; +} + +.wf-inspector-help-dl dt { + font-weight: 600; + color: var(--text-dim); +} + +.wf-inspector-help-dl dd { + margin: 0; + color: var(--text-muted); +} + .wf-field--checkbox { flex-direction: row; align-items: center; diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index 69c5017161..dc681b0401 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -53,6 +53,7 @@ import { isMobileViewport, useViewportMode } from "../hooks/useViewportMode"; import { workflowNodeTypes, type WorkflowFlowNodeData, type WorkflowEditorNodeKind } from "./nodes/WorkflowNodeTypes"; import { WorkflowEditorCatalogContext } from "./nodes/WorkflowEditorCatalogContext"; import { bareSkillName, type NodeSummaryCatalogs } from "./nodes/node-summary"; +import { nodeHelpForData } from "./nodes/node-help"; import { irToFlow, flowToIr, @@ -1968,6 +1969,8 @@ function InnerEditor({ * The structural start node needs an inspector because its entry column is editable and persisted in the workflow IR. Keep end structural-only until it has a meaningful editable property. */ const selectedNodeHasInspector = selectedNode !== null && selectedNode.data.kind !== "end"; + // FNXC:WorkflowEditor 2026-06-21-10:00: Help content for the inspector, keyed by the node's effective kind (preserved IR kind when a graph-only policy node collapsed onto a generic merge/gate/hold shape). + const selectedNodeHelp = selectedNode !== null ? nodeHelpForData(selectedNode.data) : null; const selectedEdge = edges.find((e) => e.id === selectedEdgeId) ?? null; const mobileNodeDetailStage = isMobileMode && selectedNodeHasInspector && !inspectorCollapsed; const mobileEdgeDetailStage = isMobileMode && selectedEdge !== null; @@ -3309,7 +3312,8 @@ function InnerEditor({ !(compactLayoutEnabled && !isMobileMode) && ( @@ -2669,7 +2633,6 @@ function InnerEditor({ ["add", t("workflowNodes.mobileAdd", "Add")], ["settings", t("workflowSettings.title", "Settings")], ["fields", t("workflowFields.title", "Fields")], - ["optional-steps", t("workflowOptionalSteps.title", "Optional steps")], ["columns", t("workflowColumns.title", "Columns")], ["actions", t("workflowNodes.mobileActions", "Actions")], ] as Array<[MobileWorkflowPanel, string]>).map(([panel, label]) => ( @@ -2866,16 +2829,6 @@ function InnerEditor({ )} - {mobilePanel === "optional-steps" && ( -
- p.template)} - /> -
- )} {mobilePanel === "columns" && (
diff --git a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css b/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css deleted file mode 100644 index d458de87a4..0000000000 --- a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css +++ /dev/null @@ -1,107 +0,0 @@ -/* WorkflowOptionalStepsPanel — sibling of WorkflowFieldsPanel; mirrors its layout - * so the optional-steps panel reads consistently alongside Fields/Settings. */ - -.wf-optional-steps-panel { - display: flex; - flex-direction: column; - gap: var(--space-sm); - padding: var(--space-md); -} - -.wf-optional-steps-header h3 { - margin: 0; -} - -.wf-optional-steps-hint, -.wf-optional-steps-empty { - font-size: 0.75rem; - color: var(--text-muted); - margin: 0; -} - -.wf-optional-steps-list { - list-style: none; - margin: 0; - padding: 0; - display: flex; - flex-direction: column; - gap: var(--space-sm); -} - -.wf-optional-step-item { - display: flex; - flex-direction: column; - gap: 4px; - padding: var(--space-sm); - border: 1px solid var(--border); - border-radius: var(--radius-sm, 6px); -} - -.wf-optional-step-item.is-unknown { - opacity: 0.6; -} - -.wf-optional-step-head { - display: flex; - align-items: center; - justify-content: space-between; - gap: 8px; -} - -.wf-optional-step-title { - display: inline-flex; - align-items: center; - gap: 6px; - min-width: 0; -} - -.wf-optional-step-name { - font-weight: 600; - font-size: 0.8rem; -} - -.wf-optional-step-name--unknown { - font-style: italic; - font-weight: 400; -} - -.wf-optional-step-description { - font-size: 0.72rem; - color: var(--text-muted); - margin: 0; -} - -.wf-optional-step-default { - display: inline-flex; - align-items: center; - gap: 6px; - font-size: 0.75rem; -} - -.wf-optional-step-remove { - display: inline-flex; - align-items: center; - justify-content: center; - background: transparent; - border: none; - color: var(--text-muted); - cursor: pointer; -} - -.wf-optional-step-remove:hover:not(:disabled) { - color: var(--color-error); -} - -.wf-optional-steps-add { - display: flex; - flex-direction: column; - gap: 4px; -} - -.wf-optional-steps-add-label { - display: inline-flex; - align-items: center; - gap: 4px; - font-size: 0.75rem; - color: var(--text-muted); -} diff --git a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx b/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx deleted file mode 100644 index 40bdc94a0f..0000000000 --- a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx +++ /dev/null @@ -1,177 +0,0 @@ -/** - * FNXC:WorkflowOptionalSteps 2026-06-21-00:00: - * Workflow authors need to declare which step templates are optional and set each - * one's defaultOn from the visual editor (persisted on the IR's `optionalSteps` - * array) so optional steps are authorable without hand-editing IR. - * - * WorkflowOptionalStepsPanel — the workflow editor's optional-step authoring - * surface. Sibling to {@link WorkflowFieldsPanel} / WorkflowSettingsPanel: lives - * alongside the canvas in {@link WorkflowNodeEditor} and mutates the IR's - * `optionalSteps` array through the same state/save flow (preserved across the - * round-trip by `flowToIr`). - * - * A declaration is just `{ templateId, defaultOn? }`. Display metadata - * (name/description/phase) is resolved from the built-in step-template catalog at - * render time — never duplicated into the IR — so the resolver stays the single - * source of truth. Unknown/stale template ids render a muted, still-removable row - * rather than being silently dropped. - */ -import { useCallback, useMemo } from "react"; -import { useTranslation } from "react-i18next"; -import { Plus, Trash2 } from "lucide-react"; -import { WORKFLOW_STEP_TEMPLATES, type WorkflowOptionalStep, type WorkflowStepTemplate } from "@fusion/core"; -import { phaseBadge } from "./workflow-phase-badge"; -import "./WorkflowOptionalStepsPanel.css"; - -interface WorkflowOptionalStepsPanelProps { - optionalSteps: WorkflowOptionalStep[]; - onChange: (next: WorkflowOptionalStep[]) => void; - readOnly: boolean; - /** Plugin-contributed templates, merged into the catalog when available. */ - pluginTemplates?: WorkflowStepTemplate[]; -} - -export function WorkflowOptionalStepsPanel({ - optionalSteps, - onChange, - readOnly, - pluginTemplates = [], -}: WorkflowOptionalStepsPanelProps) { - const { t } = useTranslation("app"); - - const templatesById = useMemo(() => { - const map = new Map(); - for (const tpl of [...WORKFLOW_STEP_TEMPLATES, ...pluginTemplates]) map.set(tpl.id, tpl); - return map; - }, [pluginTemplates]); - - const declaredIds = useMemo(() => new Set(optionalSteps.map((s) => s.templateId)), [optionalSteps]); - - // Catalog entries not already declared — the "Add optional step" picker source. - const available = useMemo( - () => [...templatesById.values()].filter((tpl) => !declaredIds.has(tpl.id)), - [templatesById, declaredIds], - ); - - const addStep = useCallback( - (templateId: string) => { - if (!templateId || declaredIds.has(templateId)) return; - onChange([...optionalSteps, { templateId, defaultOn: false }]); - }, - [optionalSteps, onChange, declaredIds], - ); - - const removeStep = useCallback( - (templateId: string) => onChange(optionalSteps.filter((s) => s.templateId !== templateId)), - [optionalSteps, onChange], - ); - - const toggleDefaultOn = useCallback( - (templateId: string, defaultOn: boolean) => - onChange(optionalSteps.map((s) => (s.templateId === templateId ? { ...s, defaultOn } : s))), - [optionalSteps, onChange], - ); - - return ( - - ); -} - -export default WorkflowOptionalStepsPanel; diff --git a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx index b93babdb02..f10fd6db68 100644 --- a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx +++ b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx @@ -164,13 +164,9 @@ function v2Def(): WorkflowDefinition { }; } -function v2DefWithOptional(): WorkflowDefinition { - const base = v2Def(); - return { - ...base, - ir: { ...(base.ir as object), optionalSteps: [{ templateId: "browser-verification" }] } as WorkflowDefinition["ir"], - }; -} +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: `v2DefWithOptional` and its +// optional-step DECLARATION hydration/save test are removed — the declaration +// authoring panel is retired (optional-group nodes now). function builtinDef(): WorkflowDefinition { return { @@ -751,34 +747,6 @@ describe("WorkflowNodeEditor", () => { expect(start?.column).toBe("done"); }); - it("hydrates declared optional steps and preserves them through a dirty save (round-trip)", async () => { - vi.mocked(fetchWorkflows).mockResolvedValue([v2DefWithOptional()]); - vi.mocked(updateWorkflow).mockImplementation(async (_id, updates) => ({ - ...v2DefWithOptional(), - ...(updates as object), - })); - vi.mocked(compileWorkflow).mockResolvedValue({ steps: [] }); - - render( {}} addToast={() => {}} />); - - await screen.findByText("Save"); - // The declared optional step is hydrated into the panel (optionalStepsOf). - const row = await screen.findByTestId("wf-optional-step-browser-verification"); - expect(within(row).getByText("Browser Verification")).toBeTruthy(); - - // Toggling defaultOn must mark the editor dirty (serializeGraph threading) so - // the Save button enables and persists the change. - fireEvent.click(within(row).getByRole("checkbox")); - fireEvent.click(screen.getByText("Save").closest("button")!); - - await waitFor(() => expect(updateWorkflow).toHaveBeenCalled()); - const [, updates] = vi.mocked(updateWorkflow).mock.calls[0]; - const ir = (updates as { ir: WorkflowDefinition["ir"] }).ir as { - optionalSteps?: { templateId: string; defaultOn?: boolean }[]; - }; - expect(ir.optionalSteps).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - }); - it("renders the start inspector without the entry-column select for v1 workflows", async () => { vi.mocked(fetchWorkflows).mockResolvedValue([def()]); diff --git a/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx deleted file mode 100644 index 93398b01f9..0000000000 --- a/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx +++ /dev/null @@ -1,92 +0,0 @@ -import { describe, it, expect, vi, afterEach } from "vitest"; -import { render, screen, fireEvent, cleanup, within } from "@testing-library/react"; -import { useState } from "react"; -import type { WorkflowOptionalStep } from "@fusion/core"; -import { WorkflowOptionalStepsPanel } from "../WorkflowOptionalStepsPanel"; - -// Controlled host mirroring how WorkflowNodeEditor drives the panel. -function Host({ - initial, - readOnly = false, - onState, -}: { - initial: WorkflowOptionalStep[]; - readOnly?: boolean; - onState?: (s: WorkflowOptionalStep[]) => void; -}) { - const [optionalSteps, setOptionalSteps] = useState(initial); - return ( - { - setOptionalSteps(next); - onState?.(next); - }} - /> - ); -} - -afterEach(() => { - cleanup(); - vi.clearAllMocks(); -}); - -describe("WorkflowOptionalStepsPanel", () => { - it("renders the empty state and an add picker when no steps are declared", () => { - render(); - expect(screen.getByText(/No optional steps/i)).toBeTruthy(); - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - // browser-verification is in the catalog and not yet declared → available. - expect(within(select).getByRole("option", { name: "Browser Verification" })).toBeTruthy(); - }); - - it("adds a step from the picker (defaultOn false) and removes it from the picker", () => { - const onState = vi.fn(); - render(); - fireEvent.change(screen.getByTestId("wf-optional-steps-add-select"), { - target: { value: "browser-verification" }, - }); - expect(onState).toHaveBeenCalledWith([{ templateId: "browser-verification", defaultOn: false }]); - // The declared row is shown with the resolved template name… - const row = screen.getByTestId("wf-optional-step-browser-verification"); - expect(within(row).getByText("Browser Verification")).toBeTruthy(); - // …and the picker no longer offers it. - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - expect(within(select).queryByRole("option", { name: "Browser Verification" })).toBeNull(); - }); - - it("toggles defaultOn for a declared step", () => { - const onState = vi.fn(); - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - fireEvent.click(within(row).getByRole("checkbox")); - expect(onState).toHaveBeenCalledWith([{ templateId: "browser-verification", defaultOn: true }]); - }); - - it("removes a declared step and returns it to the picker", () => { - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - fireEvent.click(within(row).getByRole("button", { name: /Remove optional step/i })); - expect(screen.queryByTestId("wf-optional-step-browser-verification")).toBeNull(); - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - expect(within(select).getByRole("option", { name: "Browser Verification" })).toBeTruthy(); - }); - - it("renders an unknown/stale templateId as a muted, still-removable row", () => { - const onState = vi.fn(); - render(); - const row = screen.getByTestId("wf-optional-step-does-not-exist"); - expect(row.className).toContain("is-unknown"); - expect(within(row).getByText(/Unknown step/i)).toBeTruthy(); - fireEvent.click(within(row).getByRole("button", { name: /Remove optional step/i })); - expect(onState).toHaveBeenCalledWith([]); - }); - - it("disables editing when readOnly", () => { - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - expect((within(row).getByRole("checkbox") as HTMLInputElement).disabled).toBe(true); - expect((within(row).getByRole("button", { name: /Remove optional step/i }) as HTMLButtonElement).disabled).toBe(true); - }); -}); diff --git a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts index 864e9ddf79..d214a4da50 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -10,7 +10,6 @@ import { fragmentSeamConflicts, copyIrWithFreshIds, columnsOf, - optionalStepsOf, columnForY, bandTop, columnsToBandNodes, @@ -1636,58 +1635,12 @@ describe("copyIrWithFreshIds", () => { }); }); -describe("optionalSteps round-trip (U2)", () => { - const v2WithOptional = (optionalSteps?: { templateId: string; defaultOn?: boolean }[]) => - makeDef( - parseWorkflowIr({ - version: "v2", - name: "wf-opt", - columns: [ - { id: "triage", name: "Triage", traits: [] }, - { id: "done", name: "Done", traits: [{ trait: "complete" }] }, - ], - nodes: [ - { id: "start", kind: "start", column: "triage" }, - { id: "end", kind: "end", column: "done" }, - ], - edges: [{ from: "start", to: "end" }], - ...(optionalSteps ? { optionalSteps } : {}), - }), - ); - - it("optionalStepsOf reads declarations from a v2 IR and returns a copy", () => { - const def = v2WithOptional([{ templateId: "browser-verification", defaultOn: true }]); - const read = optionalStepsOf(def); - expect(read).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - // mutating the result does not mutate the source IR - read[0].defaultOn = false; - expect(optionalStepsOf(def)).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - }); - - it("optionalStepsOf returns [] for v1 and for v2 without optionalSteps", () => { - const v1 = makeDef({ - version: "v1", - name: "legacy", - nodes: [ - { id: "start", kind: "start" }, - { id: "end", kind: "end" }, - ], - edges: [{ from: "start", to: "end" }], - }); - expect(optionalStepsOf(v1)).toEqual([]); - expect(optionalStepsOf(v2WithOptional())).toEqual([]); - }); - - it("flowToIr preserves optionalSteps across a full irToFlow round-trip", () => { - const def = v2WithOptional([{ templateId: "browser-verification", defaultOn: true }]); - const { nodes, edges } = irToFlow(def); - const { ir: out } = flowToIr("wf-opt", nodes, edges, columnsOf(def), [], [], optionalStepsOf(def)); - expect((out as { optionalSteps?: unknown }).optionalSteps).toEqual([ - { templateId: "browser-verification", defaultOn: true }, - ]); - }); - - it("serializes as v2 when optionalSteps present but no custom columns/fields/settings", () => { +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: +// The legacy optional-step DECLARATION authoring surface is retired: `optionalStepsOf` +// is removed and `flowToIr` no longer accepts/emits an `optionalSteps` array. Optional +// steps are graph-native `optional-group` nodes carried by the normal node/edge mapping. +describe("optionalSteps declaration authoring removed (U7)", () => { + it("flowToIr never emits a legacy optionalSteps key", () => { const { ir: out } = flowToIr( "opt-only", [ @@ -1698,21 +1651,7 @@ describe("optionalSteps round-trip (U2)", () => { [], [], [], - [{ templateId: "browser-verification" }], ); - expect(out.version).toBe("v2"); - expect((out as { optionalSteps?: unknown }).optionalSteps).toEqual([ - { templateId: "browser-verification" }, - ]); - }); - - it("omits the optionalSteps key entirely when empty (R6 byte-identity)", () => { - const def = v2WithOptional(); - const { nodes, edges } = irToFlow(def); - const { ir: out } = flowToIr("wf-opt", nodes, edges, columnsOf(def), [], [], []); expect("optionalSteps" in out).toBe(false); - // and with the arg omitted entirely - const { ir: out2 } = flowToIr("wf-opt", nodes, edges, columnsOf(def)); - expect("optionalSteps" in out2).toBe(false); }); }); diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index f889a188f2..11c718114e 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -9,7 +9,6 @@ import type { WorkflowDefinition, WorkflowFieldDefinition, WorkflowSettingDefinition, - WorkflowOptionalStep, } from "@fusion/core"; import type { WorkflowFlowNodeData, WorkflowEditorNodeKind } from "./nodes/WorkflowNodeTypes"; @@ -453,7 +452,6 @@ export function flowToIr( columns?: WorkflowIrColumn[], fields?: WorkflowFieldDefinition[], settings?: WorkflowSettingDefinition[], - optionalSteps?: WorkflowOptionalStep[], ): { ir: WorkflowIr; layout: Record } { const realNodes = nodes.filter((n) => !isColumnBandNode(n.id)); // Partition by parentId: foreach group children reassemble into that group's @@ -475,15 +473,14 @@ export function flowToIr( ); const hasFields = Array.isArray(fields) && fields.length > 0; const hasSettings = Array.isArray(settings) && settings.length > 0; - const hasOptionalSteps = Array.isArray(optionalSteps) && optionalSteps.length > 0; - // FNXC:WorkflowOptionalSteps 2026-06-21-00:00: - // Optional steps must round-trip through the node editor without data loss, yet - // must never upgrade a legacy v1 graph. Fields, settings, and optional steps are - // v2-only declarations: a workflow with any of them but no custom columns still - // serializes as v2 (with the synthesized default columns). Empty/absent → not a - // v2 signal, and the key is omitted entirely (R6 byte-identity for legacy graphs). + // FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + // The editor no longer AUTHORS legacy `optionalSteps` declarations — optional + // steps are graph-native `optional-group` nodes carried through the normal + // node/edge mapping. Fields and settings remain v2-only declarations: a workflow + // with either but no custom columns still serializes as v2 (with the synthesized + // default columns). Empty/absent → not a v2 signal (R6 byte-identity for legacy). const v2 = - (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings || hasOptionalSteps; + (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings; const layout: Record = {}; /** Project one flow node (top-level or template child) into an IR node. */ @@ -595,12 +592,6 @@ export function flowToIr( render: s.render ? { ...s.render } : undefined, })); } - if (hasOptionalSteps) { - // Optional-step DECLARATIONS round-trip through the editor opaquely (they are - // not graph nodes; the resolver + server validator are the source of truth). - // Omitted entirely when empty so legacy graphs stay byte-identical (R6). - (ir as { optionalSteps?: unknown }).optionalSteps = optionalSteps!.map((o) => ({ ...o })); - } return { ir, layout }; } @@ -1013,15 +1004,11 @@ export function settingsOf(def: WorkflowDefinition): WorkflowSettingDefinition[] })); } -/** Extract the editor's working optional-step declaration list from a definition. - * v2 with `optionalSteps` → a shallow copy; v1 or none → empty. Display metadata - * (name/icon/phase) is NOT carried here — it is resolved from the step-template - * catalog at render time so the resolver stays the single source of truth. */ -export function optionalStepsOf(def: WorkflowDefinition): WorkflowOptionalStep[] { - const ir = def.ir as { optionalSteps?: WorkflowOptionalStep[] }; - if (!isV2(def.ir) || !Array.isArray(ir.optionalSteps)) return []; - return ir.optionalSteps.map((o) => ({ ...o })); -} +/* FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + `optionalStepsOf` (the editor's legacy `optionalSteps` declaration extractor) + is removed. Optional steps are graph-native `optional-group` nodes now; the + editor reads/writes them through the normal node/edge mapping, and the per-task + toggle surfaces resolve them via `resolveWorkflowOptionalSteps`. */ /** Seed graph for a brand-new workflow: start → end with room to insert steps. */ export function emptyWorkflowIr(name: string): WorkflowIr { From 68d3c5820e938d1e99acd6cd21735c52bf37943c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 20:33:32 -0700 Subject: [PATCH 11/17] docs(FN-6880): capture optional-group toggle-id collision learning Document the code-review P1 as a logic-errors learning: a per-task graph toggle (enabledWorkflowSteps, keyed by optional-group node id) collided with the legacy step-template namespace and was silently remapped by the store resolver, bypassing an enabled group. Cross-references the per-task-override blast-radius cousins as the id-namespace-collision variant of that class. Seeds an "Optional step group" entry in CONCEPTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- CONCEPTS.md | 5 + ...toggle-id-remapped-by-step-materializer.md | 102 ++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md diff --git a/CONCEPTS.md b/CONCEPTS.md index c6990d47c7..5549f4ffa3 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -250,6 +250,11 @@ A workflow graph node that reads a declared Artifact and runs a registry parser ### Custom task field A workflow-declared, typed task field (`string | text | number | boolean | enum | multi-enum | date | url`, with enum options and render hints) whose values live in `tasks.customFields`, keyed by field id. The task model is thereby recast as core fields (title, description) + standard metadata + these workflow-defined fields. Writes pass through a single store authority (`updateTaskCustomFields`) that validates each value against the resolving workflow's schema and returns typed rejections (offending `fieldId` + `code`); agents write them via `fn_task_update`'s `custom_fields` patch. Editing a workflow's fields or switching a task's workflow orphans (never destroys) values for removed or type-incompatible ids — orphans are retained and surfaced under a detail disclosure, excluded from cards. Same id means the same field within a project; there is no cross-workflow shared field namespace. +### Optional step group +A workflow graph container node (alongside `foreach`/`loop`) whose template subgraph runs once when a task has enabled it and is bypassed otherwise — the graph-native way to make a step optional per task. Enablement is a per-task toggle set seeded from the group's workflow-level default; the group's own node id is the toggle key. It replaces the earlier execution-inert *declaration* model (a separate optional-step list run through a hidden seam), so optional steps are now real, placeable nodes rather than an out-of-graph facet. + +Single pass — no iteration or rework inside the template (this is what distinguishes it from `foreach`/`loop`). Because the toggle key is the node id, renaming or recreating a group resets its per-task enablement; and because that id may deliberately equal a built-in step-template id, the per-task enable set must keep group ids identity-stable rather than round-tripping them through legacy step-template materialization (which would remap the key and silently bypass the group). + ## Persistence & migrations ### Schema-Version Sweep diff --git a/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md b/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md new file mode 100644 index 0000000000..4221daeea7 --- /dev/null +++ b/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md @@ -0,0 +1,102 @@ +--- +title: "Optional-group enable toggle silently bypassed — node id collided with a legacy step-template namespace and was remapped" +date: 2026-06-21 +category: docs/solutions/logic-errors +module: engine (workflow store + graph executor) +problem_type: logic_error +component: service_object +symptoms: + - "Enabling a built-in optional-group (browser-verification) on a coding/stepwise task did nothing — the group's steps never ran." + - "The default-on seed path and direct graph-executor unit tests passed, masking the bug; only user-driven enable (create-with-enable or update/toggle) failed." + - "No error surfaced — the enabled group was silently bypassed." +root_cause: logic_error +resolution_type: code_fix +severity: high +related_components: + - workflow-store + - graph-executor + - optional-group +tags: + - optional-group + - enabledworkflowsteps + - per-task-override + - id-collision + - workflow-store + - silent-bypass +--- + +# Optional-group enable toggle silently bypassed — node id collided with a legacy step-template namespace and was remapped + +## Problem + +A graph-native `optional-group` workflow node is enabled per task via the `enabledWorkflowSteps` array, keyed by the group's **node id**. The graph executor runs the group only when `task.enabledWorkflowSteps.includes(node.id)`. But the store's `resolveEnabledWorkflowSteps` ran every id through the **legacy step-template materializer** (`getBuiltInWorkflowTemplate` → `ensureWorkflowStepForTemplate`). The built-in `browser-verification` group deliberately reused the template id `"browser-verification"` as its node id (for back-compat), so that id matched a `WORKFLOW_STEP_TEMPLATES` entry and was **remapped to a materialized `WorkflowStep` row id** (≠ the node id). The executor's membership check then never matched, and the enabled group was silently bypassed — the headline use case (turn the optional step on) did nothing, with no error. + +## Symptoms + +- Enabling `browser-verification` on a coding/stepwise task ran nothing pre-merge. +- Direct graph-executor tests (which pass a raw `enabledWorkflowSteps: ["browser-verification"]`) and the default-on **seed** path passed — masking the defect. +- Only the **user-driven** enable paths failed: create-with-explicit-enable and `updateTask({ enabledWorkflowSteps })` (the per-task toggle in the UI). + +## What Didn't Work + +- **Trusting the existing tests.** The unit tests used group ids like `og-on`/`og-off` that do **not** collide with any `WORKFLOW_STEP_TEMPLATES` id, so `getBuiltInWorkflowTemplate` returned undefined and the id passed through untouched — the tests were green precisely because they avoided the colliding id. The bug only fires when the group id equals a built-in template id. +- **Assuming the executor test covered it.** The two-task divergence test enabled the group by writing `enabledWorkflowSteps` straight onto the task, bypassing the store's resolver — so it never exercised the remap. The defect lived entirely in the create/update **resolution** path, one layer above the executor. + +## Solution + +Pass a workflow's optional-group node ids through `resolveEnabledWorkflowSteps` **untouched** — they are executor toggle keys, not legacy step-template ids to be materialized. + +```ts +// NEW: enumerate every optional-group node id (regardless of defaultOn). +export function resolveAllOptionalGroupIds(ir: WorkflowIr): string[] { + return resolveWorkflowOptionalSteps(ir).map((step) => step.templateId); // templateId === group node id +} + +// store.ts — the resolver gains an optional pass-through set: +private async resolveEnabledWorkflowSteps( + stepIds?: string[], + optionalGroupIds?: Set, +): Promise { + // ... + // Optional-group toggle ids pass through raw — never materialized as legacy step rows. + const template = optionalGroupIds?.has(stepId) + ? undefined + : this.getBuiltInWorkflowTemplate(stepId); + const resolvedId = template ? (await this.ensureWorkflowStepForTemplate(stepId)).id : stepId; + // ... +} + +// helper resolving the task's workflow IR → its optional-group id set: +private async optionalGroupIdSet(workflowId?: string | null): Promise> { + const wfId = workflowId ?? (await this.getDefaultWorkflowId()); + if (!wfId) return new Set(); + const def = await this.getWorkflowDefinition(wfId); + if (!def || def.kind === "fragment") return new Set(); + return new Set(resolveAllOptionalGroupIds(def.ir)); +} +``` + +Both user-enable call sites supply the set: create (`optionalGroupIdSet(input.workflowId)`) and update (`optionalGroupIdSet(getTaskWorkflowSelection(task.id)?.workflowId)`). + +**Regression test** — must use a **colliding** id (`browser-verification`), since non-colliding ids never reproduce it: create-with-enable and update/toggle both assert the raw group node id survives in `enabledWorkflowSteps`. + +## Why This Works + +The bug is a **per-task override that is read correctly at the action site but rewritten en route**. The override (`enabledWorkflowSteps`) was consulted exactly where the action runs (the graph executor), but the value was mutated in the **resolution path** before it got there, because two id namespaces overlap: graph-native optional-group **node ids** and legacy **`WorkflowStep` template ids**. The materializer is meaningful only for the retired declaration/`workflow-step`-seam execution model; for a graph-native group it is pure harm. Marking group ids as pass-through keeps the key **identity-stable** from definition through every consumer, so the executor's `includes(node.id)` check matches. + +(Verified the related slim-projection trap does **not** apply: the executor reads `enabledWorkflowSteps` off the `TaskDetail` snapshot it is handed, not a column-narrowed SELECT, so the array is fully hydrated.) + +## Prevention + +- **When introducing a new identity/key that shares a namespace with an existing one, grep every reader AND every *transformer* of that key.** A silent remap in a resolver is as fatal as a missing read — the override "survives" but as the wrong value. Demand each consumer is either re-keyed or argued identity-stable. +- **Regression tests for namespace collisions must use a *colliding* value.** A test with a deliberately distinct id proves nothing about the collision; pick the id that actually overlaps the legacy namespace (here, a built-in template id reused as a node id). +- **Test the path the user actually takes, not just the layer under test.** The executor-level test bypassed the store resolver where the bug lived; a create/update round-trip through the store would have caught it. Prefer at least one end-to-end seam test per per-task facet. +- **A facet that "works on seed/default but not on toggle" is the tell.** Asymmetry between the seed path (writes raw ids) and the user-enable path (runs the resolver) localizes the defect to the resolver. + +## Related Issues + +This is the **id-namespace-collision variant** of the per-task/per-entity override blast-radius class. Same disease (override invisible to the user, no error), different organ (key rewritten in resolution vs. not consulted at a trigger gate): + +- [Per-task auto-merge override ignored by trigger-layer gates](../logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md) — sibling: override dead from the user's perspective; theirs is a missed trigger gate, ours is a resolution-path key remap. Its "consult the override everywhere between definition and action" rule covers this case too. +- [Per-entity execution-principal override: the full blast-radius checklist](../architecture-patterns/per-entity-execution-principal-override-blast-radius.md) — the generalizing checklist; closest prior art is its "validate composite node ids against the graph, never round-trip them" example. This bug is a new bullet for that checklist. +- [Workflow-native execution through runtime primitives](../architecture-patterns/workflow-native-runtime-primitives.md) — context: the legacy-`WorkflowStep`-row vs. graph-node two-control-planes tension this collision exploits. From e4a810e9b43cabef117543a06e7dd2dc7f0ad440 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 20:47:31 -0700 Subject: [PATCH 12/17] fix(FN-6880): address PR review feedback (#1712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject failure-condition edges inside optional-group templates (the single-pass walk surfaces template failures as the group's outcome, so an internal failure edge was silently dead) — Greptile P2. - flowToIr: a container/group node (foreach/loop/optional-group) is v2-only — its presence now forces v2 serialization (an inserted optional-group on a plain workflow no longer serializes as invalid v1) — CodeRabbit. - Disabled optional-group bypass routes a plain success with no distinguishing value, so an outcome:* edge can't preempt success routing (inertness) — CodeRabbit. - Downgrade heuristic: presence of a legacy optionalSteps key (incl. []) keeps v2. - Resolver docblock corrected (config-less groups resolve to a fallback entry). - Strengthen tests: assert both inserted groups + v2 round-trip; failure-edge rejection case. - Changeset: bump to major (removed exported WorkflowOptionalStep type). - Plan: record U7a as delivered in this cohort; only the workflow-step seam infra removal remains deferred. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../retire-optional-steps-declaration.md | 4 ++- ...-workflow-optional-group-subgraphs-plan.md | 26 ++++++++++--------- .../workflow-ir-optional-group.test.ts | 7 +++++ packages/core/src/workflow-ir.ts | 23 +++++++++++++--- packages/core/src/workflow-optional-steps.ts | 6 +++-- .../__tests__/workflow-flow-mapping.test.ts | 20 +++++++++----- .../app/components/workflow-flow-mapping.ts | 7 ++++- ...-coding-browser-verification-group.test.ts | 5 ++++ .../engine/src/workflow-graph-executor.ts | 10 ++++--- 9 files changed, 78 insertions(+), 30 deletions(-) diff --git a/.changeset/retire-optional-steps-declaration.md b/.changeset/retire-optional-steps-declaration.md index 72baaa6311..3ce5035347 100644 --- a/.changeset/retire-optional-steps-declaration.md +++ b/.changeset/retire-optional-steps-declaration.md @@ -1,5 +1,7 @@ --- -"@runfusion/fusion": patch +"@runfusion/fusion": major --- +**Breaking:** the `WorkflowOptionalStep` type, previously exported from `@runfusion/fusion`, is removed — any consumer importing it must migrate to `optional-group` nodes / `ResolvedWorkflowOptionalStep`. + Retire the legacy optional-step DECLARATION model now that optional steps are graph-native `optional-group` nodes. Remove the `WorkflowOptionalStep` type and the `WorkflowIrV2.optionalSteps` IR field, drop the workflow node editor's optional-step declaration authoring panel (sidebar section, mobile tab, and collapse state), and stop threading an `optionalSteps` array through `flowToIr`/`serializeGraph`. A legacy persisted `optionalSteps` key on an old v2 workflow row is now tolerated (ignored, not validated) at parse so old rows still load as v2, and the rollback-downgrade heuristic still treats such a row as v2. The per-task optional-step toggle surfaces are unchanged — they continue to list and toggle optional steps sourced from `optional-group` nodes via `resolveWorkflowOptionalSteps` (`ResolvedWorkflowOptionalStep`). diff --git a/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md b/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md index e5e89f55bd..03d9c3650d 100644 --- a/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md +++ b/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md @@ -539,19 +539,21 @@ surfaces are enumerated: `resolveWorkflowOptionalSteps`'s output shape (source re-pointed in U3). - Step→node projection (`workflow-steps-to-ir.ts`) reused to project add-ons (U5). -### Deferred to Follow-Up Work -- **Full legacy-path retirement (U7) — deferred after execution-time scope discovery.** U1–U6 shipped and - the new model is the live path (built-ins migrated, resolver + executor on optional-group nodes). The - legacy declaration surface is now inert but **not removed**, because U7 turned out far larger than scoped: - (a) `workflow-step` is a shared `WorkflowSeam` union member woven through ~9 engine runtime files +### Delivered cohort (this PR) vs. Deferred +This PR delivers **U1–U6 plus U7a** (10 commits). U7a retired the legacy declaration *model*: the core +`WorkflowOptionalStep` type + `WorkflowIrV2.optionalSteps` field + `validateOptionalSteps`, and the editor's +declaration **authoring** surface (`WorkflowOptionalStepsPanel`, `optionalStepsOf`, the `flowToIr` +`optionalSteps` threading). A code-review pass also fixed a P1 (the optional-group toggle-id collision in +enable resolution) — captured in the commit history and in +`docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md`. The per-task toggle +surfaces (`WorkflowOptionalStepsDropdown`, inline card, modal, Workflow tab) stayed — they consume the +distinct `ResolvedWorkflowOptionalStep`. + +- **Deferred: the `workflow-step` seam infrastructure removal.** What remains of "full U7" is excising the + `workflow-step` seam itself — a shared `WorkflowSeam` union member woven through ~9 engine runtime files (`runtime-primitives`, `step-session-executor`, `workflow-node-handlers`, `active-session-registry`, - `workflow-graph-task-runner`, `executor.runWorkflowSteps`, the compiler seam-anchor), not an - optional-steps-only node — excising it is its own refactor; and (b) the dashboard still carries the prior - declaration **authoring** surface (`WorkflowOptionalStepsPanel`/`WorkflowOptionalStepsDropdown`, the - `flowToIr` `optionalSteps` threading, `optionalStepsOf`) across ~10 files. Removing the core - `WorkflowOptionalStep` type without that dashboard cleanup breaks the build. Retire both surfaces in a - focused follow-up; until then the `WorkflowOptionalStepsPanel` authors declarations the resolver no longer - reads (a known dead-authoring UI to remove with it). + `workflow-graph-task-runner`, `executor.runWorkflowSteps`, the compiler seam-anchor). It is now orphaned + (no built-in graph reaches it) but inert; excising it is its own focused refactor with its own blast radius. - **Nested/conditional groups** (an optional-group inside a split/foreach, or gated by a workflow field rather than the per-task toggle) — single-level, per-task-toggle only for now. - **Plugin-contributed add-ons as optional-group presets** beyond inserting them as flat nodes. diff --git a/packages/core/src/__tests__/workflow-ir-optional-group.test.ts b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts index 36590df04e..88806e3383 100644 --- a/packages/core/src/__tests__/workflow-ir-optional-group.test.ts +++ b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts @@ -94,6 +94,13 @@ describe("optional-group validation", () => { expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/may not contain rework edges/); }); + it("rejects failure-condition edges inside the template (single-pass bails before routing them)", () => { + const template = groupTemplate(); + // A parallel failure edge that the single-pass walk would silently never take. + template.edges.push({ from: "verify", to: "report", condition: "failure" }); + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/may not contain failure-condition edges/); + }); + it("rejects nested loop/foreach/optional-group regions", () => { const template = groupTemplate(); template.nodes.push({ diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 93b56928a3..75d96e8968 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -668,6 +668,18 @@ function validateOptionalGroup( if (isReworkEdge(edge)) { throw new WorkflowIrError(`optional-group node '${node.id}' template may not contain rework edges`); } + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: the single-pass walk + // (runOptionalGroup) surfaces a template-node failure as the GROUP's outcome + // and bails before evaluating that node's edges — so a `failure`-condition + // edge inside the template would silently never execute. Reject it as a typed + // authoring error; failure routing belongs on the group's OUTER edges. + // (Code review: Greptile P2.) + if (edge.condition === "failure") { + throw new WorkflowIrError( + `optional-group node '${node.id}' template may not contain failure-condition edges — ` + + `a template-node failure surfaces as the group's outcome and routes the group's outer failure edge`, + ); + } } const incoming = new Map(); @@ -1466,16 +1478,19 @@ export function downgradeIrToV1IfPure(ir: WorkflowIr): WorkflowIr { // Step-inversion declarations (artifacts/fields), workflow settings (U1), and // any legacy persisted optional-step declarations are v2-only features. - // FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + // FNXC:WorkflowOptionalGroup 2026-06-21-18:00 (updated 2026-06-22-09:00): // `optionalSteps` is no longer a typed IR field (retired declaration model), but // a legacy v2 row may still carry the key. Read it via an untyped cast so such a - // row is still treated as v2 (kept on v2, never silently downgraded). - const legacyOptionalSteps = (ir as { optionalSteps?: unknown[] }).optionalSteps; + // row is still treated as v2 (kept on v2, never silently downgraded). The mere + // PRESENCE of the key — including an empty `[]` — is the v2 signal: an author + // who wrote the key intended v2, and downgrading an `optionalSteps: []` row to + // v1 would still mutate its persisted shape. (Code review: CodeRabbit.) + const legacyOptionalSteps = (ir as { optionalSteps?: unknown }).optionalSteps; if ( (ir.artifacts && ir.artifacts.length > 0) || (ir.fields && ir.fields.length > 0) || (ir.settings && ir.settings.length > 0) || - (Array.isArray(legacyOptionalSteps) && legacyOptionalSteps.length > 0) + legacyOptionalSteps !== undefined ) { return ir; } diff --git a/packages/core/src/workflow-optional-steps.ts b/packages/core/src/workflow-optional-steps.ts index 4cfef149e8..f3fea182cd 100644 --- a/packages/core/src/workflow-optional-steps.ts +++ b/packages/core/src/workflow-optional-steps.ts @@ -35,8 +35,10 @@ function isOptionalGroupNode( * * Source: v2 `ir.nodes` where `kind === "optional-group"` (NOT the legacy * `ir.optionalSteps` declaration). Non-v2 graphs and graphs without any - * optional-group node resolve to `[]`. Malformed group configs are skipped so a - * stale/partial node never renders a blank UI row or breaks workflow loading. + * optional-group node resolve to `[]`. A group with a missing or partial config + * still resolves to a usable entry — `name` falls back to the node id and + * `defaultOn` to false — rather than being dropped, so a stale/partial node never + * silently disappears from the toggle UI or breaks workflow loading. * * `pluginTemplates` is accepted for signature compatibility with the prior * template-backed resolver; group nodes are self-describing, so it is currently diff --git a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts index d214a4da50..ab16bb6cab 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -1460,13 +1460,21 @@ describe("insertFragment", () => { const allIds = second.nodes.map((n) => n.id); expect(new Set(allIds).size).toBe(allIds.length); - // Round-trip: the group carries defaultOn + a single-node template. + // Round-trip: BOTH inserted groups carry defaultOn + a single-node template, + // so a regression that breaks the second insert can't pass on the first. const { ir: out } = flowToIr("wf", second.nodes, second.edges); - const og = out.nodes.find((n) => n.kind === "optional-group")!; - expect(og.config?.defaultOn).toBe(true); - const template = (og.config as { template?: { nodes: { config?: Record }[] } }).template; - expect(template?.nodes).toHaveLength(1); - expect(template?.nodes[0].config?.name).toBe("Security Audit"); + // An optional-group is a v2-only kind: its presence forces v2 serialization + // even with no columns/fields/settings, or it would serialize as v1 and fail + // parse. (Code review: CodeRabbit.) + expect(out.version).toBe("v2"); + const ogs = out.nodes.filter((n) => n.kind === "optional-group"); + expect(ogs).toHaveLength(2); + for (const og of ogs) { + expect(og.config?.defaultOn).toBe(true); + const template = (og.config as { template?: { nodes: { config?: Record }[] } }).template; + expect(template?.nodes).toHaveLength(1); + expect(template?.nodes[0].config?.name).toBe("Security Audit"); + } }); }); diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index 11c718114e..66b5b9e67d 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -479,8 +479,13 @@ export function flowToIr( // node/edge mapping. Fields and settings remain v2-only declarations: a workflow // with either but no custom columns still serializes as v2 (with the synthesized // default columns). Empty/absent → not a v2 signal (R6 byte-identity for legacy). + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: a container/group node + // (foreach/loop/optional-group) is a v2-ONLY kind — its presence must force v2, + // or an inserted optional-group on an otherwise-plain workflow would serialize + // as v1 and fail parse (validateOptionalGroup runs only on v2). (Code review: + // CodeRabbit — corroborated by the pre-merge correctness review's residual risk.) const v2 = - (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings; + (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings || groupIds.size > 0; const layout: Record = {}; /** Project one flow node (top-level or template child) into an IR node. */ diff --git a/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts b/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts index 2420815b5c..b3500f4128 100644 --- a/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts +++ b/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts @@ -88,6 +88,11 @@ describe("builtin coding browser-verification optional-group (U6)", () => { expect(result.context[`node:${GROUP_ID}:outcome`]).toBe("failure"); expect(result.visitedNodeIds).toContain(INNER_STEP_VISITED_ID); + // The group's only two outgoing edges are `success → review` and + // `failure → end`; the inner-step failure routes the failure edge, so review + // is skipped. (`end` is a terminal node the executor does not record in + // visitedNodeIds, so the routing is asserted via the group's failure outcome + // above + review being unreachable here.) expect(result.visitedNodeIds).not.toContain("review"); }); }); diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index 9dc13c158f..61bf21f3c7 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -506,10 +506,12 @@ export class WorkflowGraphExecutor { // sees "success" rather than undefined — disabled is fully inert, not // just edge-routing-inert. context[`node:${node.id}:outcome`] = "success"; - return await traverseChildren(node, { - outcome: "success", - value: "optional-group-bypassed", - }); + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: route a disabled group + // as a plain success with NO distinguishing value — a non-empty value + // could let an `outcome:*` edge preempt the success edge in + // traverseChildren, breaking the "disabled == node absent" inertness + // invariant. (Code review: CodeRabbit.) + return await traverseChildren(node, { outcome: "success" }); } const groupResult = await runOptionalGroup(node, { context, From accb32e9b63894d62259017c3884ddd008b830b3 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 00:19:15 -0700 Subject: [PATCH 13/17] fix(review): address PR #1711 review findings Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-21-003-refactor-merger-unification-u0-plan.md | 4 ++-- packages/engine/src/index.ts | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md b/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md index 385e7fc7fb..d16a26537d 100644 --- a/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md +++ b/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md @@ -23,7 +23,7 @@ It also installs the **R7 workspace merge-boundary guard** at every merge entry Merge is dispatched at `packages/engine/src/project-engine.ts:2275-2282`: -``` +```ts const mergerMode = normalizeMergerMode(settings.merger?.mode); // defaults to "ai" return mergerMode === "ai" ? runAiMerge(store, cwd, taskId, mergeOptionsWithSettings) @@ -61,7 +61,7 @@ Before claiming low blast radius, grep test fixtures, CI configs, and seeded/def ## Implementation Units > **Units `U1–U4` below are local to this plan** (they decompose master-plan U0); they are **not** the master plan's `U1–U10`. U4 (audit) may run in parallel with U1–U3. - +> > **Standing requirements:** `FNXC:Workspace ` dated comments at each non-obvious decision point (dispatch collapse, the R7 guard, the deprecation warning). A `.changeset/*.md` (`@runfusion/fusion: minor`). Respect the merge gate (`pnpm lint`, typecheck, `pnpm build`, `pnpm test:gate`) and FN-5048 (narrow seams, fake timers, no real polling / mock-the-world). **Base branch (decided):** branch off the **foundation** (`pr-1710` / `feat/workspace-multi-repo` head) — the R7 guard (U3) reads `task.workspaceWorktrees`, which the foundation adds and `main` lacks. Do **not** commit onto `pr-1710` directly; use a new branch and open a **stacked PR targeting `feat/workspace-multi-repo`** so the diff is only U0's changes. ### U1. Collapse the engine dispatch and route the two direct callers to `runAiMerge` diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 40e65e9b0f..e4cb416862 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -167,9 +167,13 @@ export { export { MeshLeaseManager, type MeshLeaseManagerOptions, type LeaseRecoveryContext } from "./mesh-lease-manager.js"; export { MissionAutopilot, type MissionAutopilotOptions } from "./mission-autopilot.js"; export { MissionExecutionLoop, type MissionExecutionLoopOptions, type ValidationResult, loopLog } from "./mission-execution-loop.js"; +// FNXC:MergerUnification 2026-06-22-00:00: @deprecated must sit on aiMergeTask's own +// export so IDE/type-aware tooling flags only aiMergeTask, not the helpers it shares with +// runAiMerge (those are NOT deprecated). A single @deprecated on the multi-member block +// would mark every symbol below as deprecated. /** @deprecated Use runAiMerge — aiMergeTask is the soft-deprecated legacy path. */ +export { aiMergeTask } from "./merger.js"; export { - aiMergeTask, listAutostashOrphans, applyAutostashBySha, dropAutostashBySha, From 95ec4bcdb6bc6d58e192a95daa8b026d3e55c656 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 02:45:46 -0700 Subject: [PATCH 14/17] docs(FN-6880): reconcile changeset + note P1 fix (review #1712) Address CodeRabbit out-of-diff review comments: the feature changeset no longer claims the legacy declaration surface "remains" (U7a retired it; the sibling changeset documents the removal), and now calls out the optional-group enable id-collision fix for release-note visibility. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/workflow-optional-group-subgraphs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/workflow-optional-group-subgraphs.md b/.changeset/workflow-optional-group-subgraphs.md index 46a98cc066..c60dad2060 100644 --- a/.changeset/workflow-optional-group-subgraphs.md +++ b/.changeset/workflow-optional-group-subgraphs.md @@ -2,4 +2,4 @@ "@runfusion/fusion": minor --- -Workflow editor: optional steps are now graph-native. A new `optional-group` container node (foreach/loop-style) holds a subgraph the executor runs once when the group is enabled for a task (per-task `enabledWorkflowSteps` + workflow `defaultOn`) and bypasses when disabled. All seven built-in add-ons (documentation-review, qa-check, security-audit, performance-review, accessibility-check, browser-verification, frontend-ux-design) are insertable from the node-editor palette as a node or wrapped in an optional-group. The built-in coding and stepwise-coding workflows now express `browser-verification` as an optional-group. The legacy declaration-based optional-steps surface remains for back-compat; its full removal is a follow-up. +Workflow editor: optional steps are now graph-native. A new `optional-group` container node (foreach/loop-style) holds a subgraph the executor runs once when the group is enabled for a task (per-task `enabledWorkflowSteps` + workflow `defaultOn`) and bypasses when disabled. All seven built-in add-ons (documentation-review, qa-check, security-audit, performance-review, accessibility-check, browser-verification, frontend-ux-design) are insertable from the node-editor palette as a node or wrapped in an optional-group. The built-in coding and stepwise-coding workflows now express `browser-verification` as an optional-group. Optional-group enable resolution correctly handles id collisions with add-on template ids, so a group's enable state is not silently bypassed during task creation/update. (The legacy declaration-based optional-steps model is retired in a sibling changeset; only the `workflow-step` seam infrastructure removal remains a follow-up.) From 7b6053900608304160d591ebc9d7299fe901e2cd Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 23 Jun 2026 13:01:06 -0700 Subject: [PATCH 15/17] FN-6953: honor unsaved ntfy config in tests Let ntfy test notifications use the current Settings form values before they are saved. - Send unsaved ntfy enabled/topic/server/token values from all Settings ntfy test buttons. - Resolve request-scoped ntfy config on the backend for general, message, and room test notifications. - Publish message and room ntfy tests directly with the resolved config instead of the active persisted notification service. - Cover unsaved ntfy test behavior across Settings UI and API routes. Files changed: .changeset/fn-6953-ntfy-test-unsaved-config.md | 5 + .../dashboard/app/components/SettingsModal.css | 5 +- .../dashboard/app/components/SettingsModal.tsx | 21 +- .../components/__tests__/SettingsModal.test.tsx | 60 +++- .../src/__tests__/routes-settings.test.ts | 181 ++++++++--- .../src/routes/register-settings-memory-routes.ts | 355 +++++++++------------ 6 files changed, 380 insertions(+), 247 deletions(-) Fusion-Task-Id: FN-6953 Fusion-Task-Lineage: 90557168-3b11-4ab4-b5c1-4261225600ba --- .../fn-6953-ntfy-test-unsaved-config.md | 5 + .../app/components/SettingsModal.css | 5 +- .../app/components/SettingsModal.tsx | 21 +- .../__tests__/SettingsModal.test.tsx | 60 ++- .../src/__tests__/routes-settings.test.ts | 181 +++++++-- .../routes/register-settings-memory-routes.ts | 357 ++++++++---------- 6 files changed, 381 insertions(+), 248 deletions(-) create mode 100644 .changeset/fn-6953-ntfy-test-unsaved-config.md diff --git a/.changeset/fn-6953-ntfy-test-unsaved-config.md b/.changeset/fn-6953-ntfy-test-unsaved-config.md new file mode 100644 index 0000000000..6371518d61 --- /dev/null +++ b/.changeset/fn-6953-ntfy-test-unsaved-config.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix ntfy test notifications to honor unsaved Settings form config so users can enable ntfy, enter a valid topic/server/token, and send a test notification before saving. diff --git a/packages/dashboard/app/components/SettingsModal.css b/packages/dashboard/app/components/SettingsModal.css index b5d7af8c75..8ab9bd73e1 100644 --- a/packages/dashboard/app/components/SettingsModal.css +++ b/packages/dashboard/app/components/SettingsModal.css @@ -696,13 +696,16 @@ The embedded title reads like other embedded-view titles (Planning modal-header- } } +/* +FNXC:SettingsMobile 2026-06-23-09:02: +Settings section headings should preserve hierarchy through spacing and type only. Avoid per-heading divider borders so mobile and desktop shared Settings sections keep the lighter scrollbar-focused chrome contract. +*/ .settings-section-heading { font-size: 14px; font-weight: 600; padding: var(--space-lg) 0 var(--space-md); margin: 0 0 var(--space-md); color: var(--text); - border-bottom: 1px solid var(--border); } /* First heading inside the section drops top padding to remove a redundant diff --git a/packages/dashboard/app/components/SettingsModal.tsx b/packages/dashboard/app/components/SettingsModal.tsx index ee7f96253a..28d6d01ed3 100644 --- a/packages/dashboard/app/components/SettingsModal.tsx +++ b/packages/dashboard/app/components/SettingsModal.tsx @@ -1851,17 +1851,22 @@ export function SettingsModal({ return next; }); try { + /* + FNXC:Notifications 2026-06-23-08:49: + Settings notification tests must send the current unsaved ntfy form values for every ntfy test affordance. Users validate the exact topic/server/token they just typed before saving, so message/room test requests carry the same request-scoped config as the general ntfy test. + */ + const currentNtfyConfig = { + ntfyEnabled: form.ntfyEnabled, + ntfyTopic: form.ntfyTopic, + ...(form.ntfyBaseUrl?.trim() ? { ntfyBaseUrl: form.ntfyBaseUrl.trim() } : {}), + ...(form.ntfyAccessToken?.trim() ? { ntfyAccessToken: form.ntfyAccessToken.trim() } : {}), + }; const config = providerId === "ntfy" - ? { - ntfyEnabled: form.ntfyEnabled, - ntfyTopic: form.ntfyTopic, - ...(form.ntfyBaseUrl?.trim() ? { ntfyBaseUrl: form.ntfyBaseUrl.trim() } : {}), - ...(form.ntfyAccessToken?.trim() ? { ntfyAccessToken: form.ntfyAccessToken.trim() } : {}), - } + ? currentNtfyConfig : providerId === "ntfy-message" - ? { messageEventType: "message:agent-to-user" } + ? { ...currentNtfyConfig, messageEventType: "message:agent-to-user" } : providerId === "ntfy-room" - ? { messageEventType: "message:room" } + ? { ...currentNtfyConfig, messageEventType: "message:room" } : { webhookUrl: form.webhookUrl, webhookFormat: form.webhookFormat || "generic", diff --git a/packages/dashboard/app/components/__tests__/SettingsModal.test.tsx b/packages/dashboard/app/components/__tests__/SettingsModal.test.tsx index 6b9602f3c2..2313590734 100644 --- a/packages/dashboard/app/components/__tests__/SettingsModal.test.tsx +++ b/packages/dashboard/app/components/__tests__/SettingsModal.test.tsx @@ -4895,6 +4895,54 @@ describe("SettingsModal", () => { }); }); + it("sends unsaved ntfy form config before saving", async () => { + mockFetchSettings.mockResolvedValueOnce({ ...defaultSettings, ntfyEnabled: false, ntfyTopic: undefined }); + renderModal(); + await waitForSettingsModalReady(); + await openNotificationsSection(); + + await user.click(screen.getByLabelText("Enable")); + await user.type(screen.getByLabelText("ntfy Topic"), "fresh-topic"); + await user.click(screen.getByText("Advanced")); + await user.type(screen.getByLabelText("Custom ntfy server URL (optional)"), "https://ntfy.override.example//"); + await user.type(screen.getByLabelText("Access token (optional)"), "override-token"); + await user.click(screen.getByRole("button", { name: /Test notification/ })); + + await waitFor(() => { + expect(mockTestNotification).toHaveBeenCalledWith( + "ntfy", + expect.objectContaining({ + ntfyEnabled: true, + ntfyTopic: "fresh-topic", + ntfyBaseUrl: "https://ntfy.override.example//", + ntfyAccessToken: "override-token", + }), + undefined, + ); + }); + expect(mockUpdateSettings).not.toHaveBeenCalled(); + expect(mockUpdateGlobalSettings).not.toHaveBeenCalled(); + }); + + it("keeps ntfy test disabled until the current form has a valid topic", async () => { + mockFetchSettings.mockResolvedValueOnce({ ...defaultSettings, ntfyEnabled: false, ntfyTopic: undefined }); + renderModal(); + await waitForSettingsModalReady(); + await openNotificationsSection(); + + await user.click(screen.getByLabelText("Enable")); + const testButton = screen.getByRole("button", { name: /Test notification/ }); + expect(testButton).toBeDisabled(); + + await user.type(screen.getByLabelText("ntfy Topic"), "bad topic!"); + expect(testButton).toBeDisabled(); + expect(mockTestNotification).not.toHaveBeenCalled(); + + await user.clear(screen.getByLabelText("ntfy Topic")); + await user.type(screen.getByLabelText("ntfy Topic"), "fresh-topic"); + expect(testButton).toBeEnabled(); + }); + it("clears a saved ntfy access token via global null-as-delete semantics", async () => { mockFetchSettings.mockResolvedValueOnce({ ...defaultSettings, @@ -4929,7 +4977,11 @@ describe("SettingsModal", () => { await waitFor(() => { expect(mockTestNotification).toHaveBeenCalledWith( "ntfy", - { messageEventType: "message:agent-to-user" }, + expect.objectContaining({ + messageEventType: "message:agent-to-user", + ntfyEnabled: true, + ntfyTopic: "test-topic", + }), undefined, ); }); @@ -4953,7 +5005,11 @@ describe("SettingsModal", () => { await waitFor(() => { expect(mockTestNotification).toHaveBeenCalledWith( "ntfy", - { messageEventType: "message:room" }, + expect.objectContaining({ + messageEventType: "message:room", + ntfyEnabled: true, + ntfyTopic: "test-topic", + }), undefined, ); }); diff --git a/packages/dashboard/src/__tests__/routes-settings.test.ts b/packages/dashboard/src/__tests__/routes-settings.test.ts index 6bff37c484..dc07d77c17 100644 --- a/packages/dashboard/src/__tests__/routes-settings.test.ts +++ b/packages/dashboard/src/__tests__/routes-settings.test.ts @@ -1772,6 +1772,37 @@ describe("POST /settings/test-ntfy", () => { expect(url).toBe("https://ntfy.override.example/my-topic"); }); + it("uses unsaved request ntfy config when saved settings are disabled", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ + ntfyEnabled: false, + ntfyTopic: undefined, + ntfyBaseUrl: "https://ntfy.saved.example", + ntfyAccessToken: "saved-token", + }); + + const res = await REQUEST( + buildApp(), + "POST", + "/api/settings/test-ntfy", + JSON.stringify({ + ntfyEnabled: true, + ntfyTopic: "fresh-topic", + ntfyBaseUrl: "https://ntfy.override.example//", + ntfyAccessToken: "override-token", + }), + { "content-type": "application/json" }, + ); + + expect(res.status).toBe(200); + expect(store.updateSettings).not.toHaveBeenCalled(); + expect(store.updateGlobalSettings).not.toHaveBeenCalled(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const url = fetchSpy.mock.calls[0]?.[0] as string; + const options = fetchSpy.mock.calls[0]?.[1] as RequestInit; + expect(url).toBe("https://ntfy.override.example/fresh-topic"); + expect(options.headers).toHaveProperty("Authorization", "Bearer override-token"); + }); + it("falls back to saved ntfyBaseUrl when request override is blank", async () => { (store.getSettings as ReturnType).mockResolvedValue({ ntfyEnabled: true, @@ -1973,69 +2004,82 @@ describe("POST /settings/test-notification", () => { ); }); - it("ntfy provider dispatches a message-event pipeline test when messageEventType is provided", async () => { - const dispatchSpy = vi.fn().mockResolvedValue(undefined); - mockGetActiveNotificationService.mockReturnValue({ dispatch: dispatchSpy }); + it("ntfy provider sends a message-event test with unsaved config when messageEventType is provided", async () => { (store.getSettings as ReturnType).mockResolvedValue({ - ntfyEnabled: true, - ntfyTopic: "test-topic", + ntfyEnabled: false, + ntfyTopic: "saved-topic", + ntfyBaseUrl: "https://ntfy.saved.example", + ntfyAccessToken: "saved-token", }); const res = await REQUEST( buildApp(), "POST", "/api/settings/test-notification", - JSON.stringify({ providerId: "ntfy", messageEventType: "message:agent-to-user" }), + JSON.stringify({ + providerId: "ntfy", + config: { + messageEventType: "message:agent-to-user", + ntfyEnabled: true, + ntfyTopic: "fresh-message-topic", + ntfyBaseUrl: "https://ntfy.message.example//", + ntfyAccessToken: "message-token", + }, + }), { "content-type": "application/json" }, ); expect(res.status).toBe(200); expect(res.body).toEqual({ success: true }); - expect(dispatchSpy).toHaveBeenCalledWith( - "message:agent-to-user", - expect.objectContaining({ - event: "message:agent-to-user", - metadata: expect.objectContaining({ - fromId: "system", - toId: "user", - preview: "Fusion test message notification", - }), - }), - ); - expect(fetchSpy).not.toHaveBeenCalled(); + expect(mockGetActiveNotificationService).not.toHaveBeenCalled(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const options = fetchSpy.mock.calls[0]?.[1] as RequestInit; + expect(fetchSpy.mock.calls[0]?.[0]).toBe("https://ntfy.message.example/fresh-message-topic"); + expect(options.headers).toMatchObject({ + Title: "New message from Fusion", + Priority: "high", + Authorization: "Bearer message-token", + }); + expect(options.body).toBe("Fusion → you: Fusion test message notification"); }); - it("ntfy provider dispatches a room message-event pipeline test when messageEventType is message:room", async () => { - const dispatchSpy = vi.fn().mockResolvedValue(undefined); - mockGetActiveNotificationService.mockReturnValue({ dispatch: dispatchSpy }); + it("ntfy provider sends a room message-event test with unsaved config when messageEventType is message:room", async () => { (store.getSettings as ReturnType).mockResolvedValue({ - ntfyEnabled: true, - ntfyTopic: "test-topic", + ntfyEnabled: false, + ntfyTopic: "saved-topic", + ntfyBaseUrl: "https://ntfy.saved.example", + ntfyAccessToken: "saved-token", }); const res = await REQUEST( buildApp(), "POST", "/api/settings/test-notification", - JSON.stringify({ providerId: "ntfy", messageEventType: "message:room" }), + JSON.stringify({ + providerId: "ntfy", + config: { + messageEventType: "message:room", + ntfyEnabled: true, + ntfyTopic: "fresh-room-topic", + ntfyBaseUrl: "https://ntfy.room.example//", + ntfyAccessToken: "room-token", + }, + }), { "content-type": "application/json" }, ); expect(res.status).toBe(200); expect(res.body).toEqual({ success: true }); - expect(dispatchSpy).toHaveBeenCalledWith( - "message:room", - expect.objectContaining({ - event: "message:room", - metadata: expect.objectContaining({ - roomId: "test-room", - roomName: "Test Room", - senderName: "Fusion", - preview: "Fusion test room notification", - }), - }), - ); - expect(fetchSpy).not.toHaveBeenCalled(); + expect(mockGetActiveNotificationService).not.toHaveBeenCalled(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const options = fetchSpy.mock.calls[0]?.[1] as RequestInit; + expect(fetchSpy.mock.calls[0]?.[0]).toBe("https://ntfy.room.example/fresh-room-topic"); + expect(options.headers).toMatchObject({ + Title: "#Test Room — Fusion", + Priority: "default", + Authorization: "Bearer room-token", + }); + expect(options.body).toBe("Fusion in #Test Room: Fusion test room notification"); }); it("ntfy provider uses config override for baseUrl", async () => { @@ -2057,6 +2101,69 @@ describe("POST /settings/test-notification", () => { expect(fetchSpy.mock.calls[0]?.[0]).toBe("https://ntfy.override.example/my-topic"); }); + it("ntfy provider sends with unsaved config when saved settings are disabled", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ + ntfyEnabled: false, + ntfyTopic: undefined, + ntfyBaseUrl: "https://ntfy.saved.example", + ntfyAccessToken: "saved-token", + }); + + const res = await REQUEST( + buildApp(), + "POST", + "/api/settings/test-notification", + JSON.stringify({ + providerId: "ntfy", + config: { + ntfyEnabled: true, + ntfyTopic: "fresh-topic", + ntfyBaseUrl: "https://ntfy.override.example//", + ntfyAccessToken: "override-token", + }, + }), + { "content-type": "application/json" }, + ); + + expect(res.status).toBe(200); + expect(store.updateSettings).not.toHaveBeenCalled(); + expect(store.updateGlobalSettings).not.toHaveBeenCalled(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const options = fetchSpy.mock.calls[0]?.[1] as RequestInit; + expect(fetchSpy.mock.calls[0]?.[0]).toBe("https://ntfy.override.example/fresh-topic"); + expect(options.headers).toHaveProperty("Authorization", "Bearer override-token"); + }); + + it("ntfy provider ignores blank request baseUrl and token overrides", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ + ntfyEnabled: true, + ntfyTopic: "saved-topic", + ntfyBaseUrl: "https://ntfy.saved.example", + ntfyAccessToken: "saved-token", + }); + + const res = await REQUEST( + buildApp(), + "POST", + "/api/settings/test-notification", + JSON.stringify({ + providerId: "ntfy", + config: { + ntfyEnabled: true, + ntfyTopic: "fresh-topic", + ntfyBaseUrl: " ", + ntfyAccessToken: " ", + }, + }), + { "content-type": "application/json" }, + ); + + expect(res.status).toBe(200); + const options = fetchSpy.mock.calls[0]?.[1] as RequestInit; + expect(fetchSpy.mock.calls[0]?.[0]).toBe("https://ntfy.saved.example/fresh-topic"); + expect(options.headers).toHaveProperty("Authorization", "Bearer saved-token"); + }); + it("ntfy provider sends Authorization header from saved or override token", async () => { (store.getSettings as ReturnType).mockResolvedValue({ ntfyEnabled: true, diff --git a/packages/dashboard/src/routes/register-settings-memory-routes.ts b/packages/dashboard/src/routes/register-settings-memory-routes.ts index 59850ccca4..e8205ab684 100644 --- a/packages/dashboard/src/routes/register-settings-memory-routes.ts +++ b/packages/dashboard/src/routes/register-settings-memory-routes.ts @@ -47,7 +47,6 @@ import { import { buildSessionSkillContextSync, createFnAgent as engineCreateFnAgent, - getActiveNotificationService, probeWorktrunk, resolveWorktrunkBinary, } from "@fusion/engine"; @@ -2044,84 +2043,160 @@ export function registerSettingsMemoryRoutes(ctx: ApiRoutesContext, deps: Settin * Returns the user's global pi extension settings from ~/.pi/agent/settings.json. * Includes packages, extension paths, skill paths, prompt template paths, and theme paths. */ - router.post("/settings/test-ntfy", async (req, res) => { - const normalizeNtfyBaseUrl = (value: string, source: "request" | "settings"): string => { - const trimmed = value.trim(); - if (!trimmed) { - throw badRequest("ntfy server URL cannot be empty"); - } + const normalizeHttpUrl = (value: string, fieldName: string): string => { + const trimmed = value.trim(); + if (!trimmed) { + throw badRequest(`${fieldName} cannot be empty`); + } - let parsed: URL; - try { - parsed = new URL(trimmed); - } catch { - throw badRequest(`ntfy server URL from ${source} must be a valid URL`); - } + let parsed: URL; + try { + parsed = new URL(trimmed); + } catch { + throw badRequest(`${fieldName} must be a valid URL`); + } - if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { - throw badRequest("ntfy server URL must use http:// or https://"); - } + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + throw badRequest(`${fieldName} must use http:// or https://`); + } - return trimmed.replace(/\/+$/, ""); + return trimmed; + }; + + const normalizeNtfyBaseUrl = (value: string, source: "request" | "settings"): string => { + const normalized = normalizeHttpUrl(value, `ntfy server URL from ${source}`); + return normalized.replace(/\/+$/, ""); + }; + + const getOwnValue = (source: Record, key: string): unknown => ( + Object.prototype.hasOwnProperty.call(source, key) ? source[key] : undefined + ); + + const getRequestNtfyValue = (body: Record, config: Record, key: string): unknown => { + const configValue = getOwnValue(config, key); + return configValue !== undefined ? configValue : getOwnValue(body, key); + }; + + type NtfyTestMessageEventType = "message:agent-to-user" | "message:agent-to-agent" | "message:room"; + + function resolveEffectiveNtfyTestConfig( + settings: Record, + body: Record, + config: Record = {}, + ): { topic: string; ntfyBaseUrl: string; ntfyAccessToken?: string } { + /* + FNXC:Notifications 2026-06-23-08:34: + Test sends must honor unsaved Settings form state because users enable ntfy, enter a topic/server/token, and test before saving. Resolve request-scoped values ahead of persisted settings without persisting or logging tokens. + + FNXC:Notifications 2026-06-23-10:21: + Every ntfy test affordance, including message and room tests, must publish with the request-scoped topic/server/token instead of the active notification service's persisted provider state. + */ + const enabledOverride = getRequestNtfyValue(body, config, "ntfyEnabled"); + if (enabledOverride !== undefined && enabledOverride !== null && typeof enabledOverride !== "boolean") { + throw badRequest("ntfy enabled must be a boolean"); + } + const ntfyEnabled = typeof enabledOverride === "boolean" ? enabledOverride : settings.ntfyEnabled === true; + if (!ntfyEnabled) { + throw badRequest("ntfy notifications are not enabled"); + } + + const topicOverride = getRequestNtfyValue(body, config, "ntfyTopic"); + if (topicOverride !== undefined && topicOverride !== null && typeof topicOverride !== "string") { + throw badRequest("ntfy topic must be a string"); + } + const topic = typeof topicOverride === "string" ? topicOverride : settings.ntfyTopic; + if (typeof topic !== "string" || !/^[a-zA-Z0-9_-]{1,64}$/.test(topic)) { + throw badRequest("ntfy topic is not configured or invalid"); + } + + const baseUrlOverride = getRequestNtfyValue(body, config, "ntfyBaseUrl"); + if (baseUrlOverride !== undefined && baseUrlOverride !== null && typeof baseUrlOverride !== "string") { + throw badRequest("ntfy server URL must be a string"); + } + const requestBaseUrl = typeof baseUrlOverride === "string" && baseUrlOverride.trim() + ? normalizeNtfyBaseUrl(baseUrlOverride, "request") + : undefined; + const storedBaseUrl = typeof settings.ntfyBaseUrl === "string" && settings.ntfyBaseUrl.trim() + ? normalizeNtfyBaseUrl(settings.ntfyBaseUrl, "settings") + : undefined; + + const tokenOverride = getRequestNtfyValue(body, config, "ntfyAccessToken"); + if (tokenOverride !== undefined && tokenOverride !== null && typeof tokenOverride !== "string") { + throw badRequest("ntfy access token must be a string"); + } + const requestToken = typeof tokenOverride === "string" && tokenOverride.trim() + ? tokenOverride.trim() + : undefined; + const storedToken = typeof settings.ntfyAccessToken === "string" && settings.ntfyAccessToken.trim() + ? settings.ntfyAccessToken.trim() + : undefined; + + return { + topic, + ntfyBaseUrl: requestBaseUrl ?? storedBaseUrl ?? "https://ntfy.sh", + ntfyAccessToken: requestToken ?? storedToken, }; + } + + async function sendNtfyTestNotification( + options: { topic: string; ntfyBaseUrl: string; ntfyAccessToken?: string; messageEventType?: NtfyTestMessageEventType }, + ): Promise { + const contentByEvent: Record = { + default: { + title: "Fusion test notification", + message: "Fusion test notification — your notifications are working!", + priority: "default", + }, + "message:agent-to-user": { + title: "New message from Fusion", + message: "Fusion → you: Fusion test message notification", + priority: "high", + }, + "message:agent-to-agent": { + title: "Fusion → recipient", + message: "Fusion messaged recipient: Fusion test message notification", + priority: "default", + }, + "message:room": { + title: "#Test Room — Fusion", + message: "Fusion in #Test Room: Fusion test room notification", + priority: "default", + }, + }; + const content = contentByEvent[options.messageEventType ?? "default"]; + const headers: Record = { + Title: content.title, + Priority: content.priority, + "Content-Type": "text/plain", + }; + if (options.ntfyAccessToken) { + headers.Authorization = `Bearer ${options.ntfyAccessToken}`; + } + + const response = await fetch(`${options.ntfyBaseUrl}/${options.topic}`, { + method: "POST", + headers, + body: content.message, + }); + + if (!response.ok) { + throw new ApiError(502, `ntfy server returned ${response.status}: ${response.statusText}`); + } + } + + router.post("/settings/test-ntfy", async (req, res) => { try { + const body = (req.body ?? {}) as Record; + const configValue = body.config; + if (configValue !== undefined && (typeof configValue !== "object" || configValue === null || Array.isArray(configValue))) { + throw badRequest("config must be an object when provided"); + } + const config = (configValue ?? {}) as Record; const { store: scopedStore } = await getProjectContext(req); const settings = await scopedStore.getSettings(); - - // Validate ntfy is enabled - if (!settings.ntfyEnabled) { - throw badRequest("ntfy notifications are not enabled"); - } - - // Validate topic exists and matches required format - const topic = settings.ntfyTopic; - if (!topic || !/^[a-zA-Z0-9_-]{1,64}$/.test(topic)) { - throw badRequest("ntfy topic is not configured or invalid"); - } - - const overrideValue = req.body?.ntfyBaseUrl; - if (overrideValue !== undefined && overrideValue !== null && typeof overrideValue !== "string") { - throw badRequest("ntfy server URL must be a string"); - } - - const requestOverride = typeof overrideValue === "string" && overrideValue.trim() - ? normalizeNtfyBaseUrl(overrideValue, "request") - : undefined; - const storedServer = typeof settings.ntfyBaseUrl === "string" && settings.ntfyBaseUrl.trim() - ? normalizeNtfyBaseUrl(settings.ntfyBaseUrl, "settings") - : undefined; - const tokenOverride = req.body?.ntfyAccessToken; - if (tokenOverride !== undefined && tokenOverride !== null && typeof tokenOverride !== "string") { - throw badRequest("ntfy access token must be a string"); - } - const requestToken = typeof tokenOverride === "string" && tokenOverride.trim() - ? tokenOverride.trim() - : undefined; - const storedToken = typeof settings.ntfyAccessToken === "string" && settings.ntfyAccessToken.trim() - ? settings.ntfyAccessToken.trim() - : undefined; - const ntfyBaseUrl = requestOverride ?? storedServer ?? "https://ntfy.sh"; - const url = `${ntfyBaseUrl}/${topic}`; - const headers: Record = { - "Title": "Fusion test notification", - "Priority": "default", - "Content-Type": "text/plain", - }; - const ntfyAccessToken = requestToken ?? storedToken; - if (ntfyAccessToken) { - headers.Authorization = `Bearer ${ntfyAccessToken}`; - } - - const response = await fetch(url, { - method: "POST", - headers, - body: "Fusion test notification — your notifications are working!", - }); - - if (!response.ok) { - throw new ApiError(502, `ntfy server returned ${response.status}: ${response.statusText}`); - } + const configForTest = resolveEffectiveNtfyTestConfig(settings as Record, body, config); + await sendNtfyTestNotification(configForTest); res.json({ success: true }); } catch (err: unknown) { @@ -2133,31 +2208,6 @@ export function registerSettingsMemoryRoutes(ctx: ApiRoutesContext, deps: Settin }); router.post("/settings/test-notification", async (req, res) => { - const normalizeHttpUrl = (value: string, fieldName: string): string => { - const trimmed = value.trim(); - if (!trimmed) { - throw badRequest(`${fieldName} cannot be empty`); - } - - let parsed: URL; - try { - parsed = new URL(trimmed); - } catch { - throw badRequest(`${fieldName} must be a valid URL`); - } - - if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { - throw badRequest(`${fieldName} must use http:// or https://`); - } - - return trimmed; - }; - - const normalizeNtfyBaseUrl = (value: string, source: "request" | "settings"): string => { - const normalized = normalizeHttpUrl(value, `ntfy server URL from ${source}`); - return normalized.replace(/\/+$/, ""); - }; - try { const body = (req.body ?? {}) as Record; const providerId = body.providerId; @@ -2176,113 +2226,20 @@ export function registerSettingsMemoryRoutes(ctx: ApiRoutesContext, deps: Settin if (providerId === "ntfy") { const requestedMessageEventType = config.messageEventType ?? body.messageEventType; - if (requestedMessageEventType !== undefined) { - if ( - requestedMessageEventType !== "message:agent-to-user" - && requestedMessageEventType !== "message:agent-to-agent" - && requestedMessageEventType !== "message:room" - ) { - throw badRequest("messageEventType must be message:agent-to-user, message:agent-to-agent, or message:room"); - } - - const notificationService = getActiveNotificationService(); - if (!notificationService) { - throw new ApiError(502, "Notification service is not active"); - } - - try { - const messageId = `test-${crypto.randomUUID()}`; - if (requestedMessageEventType === "message:room") { - await notificationService.dispatch(requestedMessageEventType, { - taskId: undefined, - taskTitle: undefined, - event: requestedMessageEventType, - metadata: { - messageId, - roomId: "test-room", - roomName: "Test Room", - senderAgentId: "system", - senderName: "Fusion", - preview: "Fusion test room notification", - type: "room-assistant", - }, - }); - } else { - const messageType = requestedMessageEventType.split(":")[1] ?? "agent-to-user"; - await notificationService.dispatch(requestedMessageEventType, { - taskId: undefined, - taskTitle: undefined, - event: requestedMessageEventType, - metadata: { - messageId, - fromId: "system", - fromType: "agent", - toId: "user", - toType: "user", - type: messageType, - preview: "Fusion test message notification", - }, - }); - } - res.json({ success: true }); - return; - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - throw new ApiError(502, `Failed to dispatch message notification: ${message}`); - } + if ( + requestedMessageEventType !== undefined + && requestedMessageEventType !== "message:agent-to-user" + && requestedMessageEventType !== "message:agent-to-agent" + && requestedMessageEventType !== "message:room" + ) { + throw badRequest("messageEventType must be message:agent-to-user, message:agent-to-agent, or message:room"); } - if (!settings.ntfyEnabled) { - throw badRequest("ntfy notifications are not enabled"); - } - - const topic = settings.ntfyTopic; - if (!topic || !/^[a-zA-Z0-9_-]{1,64}$/.test(topic)) { - throw badRequest("ntfy topic is not configured or invalid"); - } - - const overrideValue = config.ntfyBaseUrl ?? body.ntfyBaseUrl; - if (overrideValue !== undefined && overrideValue !== null && typeof overrideValue !== "string") { - throw badRequest("ntfy server URL must be a string"); - } - - const requestOverride = typeof overrideValue === "string" && overrideValue.trim() - ? normalizeNtfyBaseUrl(overrideValue, "request") - : undefined; - const storedServer = typeof settings.ntfyBaseUrl === "string" && settings.ntfyBaseUrl.trim() - ? normalizeNtfyBaseUrl(settings.ntfyBaseUrl, "settings") - : undefined; - const tokenOverride = config.ntfyAccessToken ?? body.ntfyAccessToken; - if (tokenOverride !== undefined && tokenOverride !== null && typeof tokenOverride !== "string") { - throw badRequest("ntfy access token must be a string"); - } - const requestToken = typeof tokenOverride === "string" && tokenOverride.trim() - ? tokenOverride.trim() - : undefined; - const storedToken = typeof settings.ntfyAccessToken === "string" && settings.ntfyAccessToken.trim() - ? settings.ntfyAccessToken.trim() - : undefined; - const ntfyBaseUrl = requestOverride ?? storedServer ?? "https://ntfy.sh"; - const url = `${ntfyBaseUrl}/${topic}`; - const headers: Record = { - "Title": "Fusion test notification", - "Priority": "default", - "Content-Type": "text/plain", - }; - const ntfyAccessToken = requestToken ?? storedToken; - if (ntfyAccessToken) { - headers.Authorization = `Bearer ${ntfyAccessToken}`; - } - - const response = await fetch(url, { - method: "POST", - headers, - body: "Fusion test notification — your notifications are working!", + const configForTest = resolveEffectiveNtfyTestConfig(settings as Record, body, config); + await sendNtfyTestNotification({ + ...configForTest, + messageEventType: requestedMessageEventType as NtfyTestMessageEventType | undefined, }); - if (!response.ok) { - throw new ApiError(502, `ntfy server returned ${response.status}: ${response.statusText}`); - } - res.json({ success: true }); return; } From e9a6955b497e868626b6deb9d95a49fd841e33f1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 23 Jun 2026 14:01:19 -0700 Subject: [PATCH 16/17] FN-6939: add narrow dock preview modal Adds an accessible preview modal path for constrained Dev Server right-dock layouts. - Detect narrow direct right-dock hosts while preserving inline previews for full-page, mobile, and expanded modal hosts. - Replace the crowded inline preview with a compact Open preview launcher and accessible modal controls in narrow docks. - Add focused preview/mobile coverage, documentation, styling, and a published package changeset. Files changed: .../fn-6939-dev-server-narrow-preview-modal.md | 5 + docs/dashboard-guide.md | 3 + .../dashboard/app/components/DevServerView.css | 100 ++++- .../dashboard/app/components/DevServerView.tsx | 449 +++++++++++++++------ .../__tests__/DevServerView.mobile.test.tsx | 17 +- .../__tests__/DevServerView.preview.test.tsx | 148 +++++++ 6 files changed, 603 insertions(+), 119 deletions(-) Fusion-Task-Id: FN-6939 Fusion-Task-Lineage: 745ea56d-16bf-4246-bfe7-0461754466d9 --- ...fn-6939-dev-server-narrow-preview-modal.md | 5 + docs/dashboard-guide.md | 3 + .../app/components/DevServerView.css | 100 +++- .../app/components/DevServerView.tsx | 447 +++++++++++++----- .../__tests__/DevServerView.mobile.test.tsx | 17 +- .../__tests__/DevServerView.preview.test.tsx | 148 ++++++ 6 files changed, 602 insertions(+), 118 deletions(-) create mode 100644 .changeset/fn-6939-dev-server-narrow-preview-modal.md diff --git a/.changeset/fn-6939-dev-server-narrow-preview-modal.md b/.changeset/fn-6939-dev-server-narrow-preview-modal.md new file mode 100644 index 0000000000..2133d343fb --- /dev/null +++ b/.changeset/fn-6939-dev-server-narrow-preview-modal.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix narrow right-sidebar Dev Server preview overlap by replacing the inline preview with an accessible modal launcher when the dock is very narrow, while keeping inline preview for full-page, mobile viewport, and expanded pop-out hosts. diff --git a/docs/dashboard-guide.md b/docs/dashboard-guide.md index de1e9379d9..0ce4b15586 100644 --- a/docs/dashboard-guide.md +++ b/docs/dashboard-guide.md @@ -866,6 +866,9 @@ Features: - Start, stop, and restart the current server session - Manage preview URLs with embedded preview and **Open in new tab** fallback - Tail live logs, load older history, and refresh session status +- When Dev Server is hosted in a very narrow right sidebar, open the preview from the compact **Open preview** launcher; the modal keeps preview actions available while configuration and logs stay usable in the sidebar. + + For module-level behavior and API surfaces, see [Dev Server modules](./dev-server-modules.md). diff --git a/packages/dashboard/app/components/DevServerView.css b/packages/dashboard/app/components/DevServerView.css index 5bb722fc38..437f83087e 100644 --- a/packages/dashboard/app/components/DevServerView.css +++ b/packages/dashboard/app/components/DevServerView.css @@ -582,6 +582,65 @@ exactly when the surrounding chrome is gone. margin: 0; } +.devserver-preview-modal-launcher { + align-items: stretch; +} + +.devserver-preview-modal-launcher__copy { + display: flex; + align-items: center; + gap: var(--space-sm); + min-width: 0; +} + +.devserver-preview-modal-launcher__copy .devserver-preview-url-badge { + max-width: none; +} + +.devserver-preview-modal-launcher__description { + margin: 0; + color: var(--text-muted); + line-height: 1.5; +} + +.devserver-preview-modal-overlay { + align-items: center; + padding: var(--space-xl); +} + +.devserver-preview-modal { + width: min(calc(var(--space-2xl) * 28), calc(100vw - var(--space-xl) * 2)); + max-height: calc(100vh - var(--space-xl) * 2); +} + +.devserver-preview-modal__titlebar { + display: flex; + align-items: center; + justify-content: space-between; + gap: var(--space-md); + padding: var(--space-md); + border-bottom: 1px solid var(--border); +} + +.devserver-preview-modal__titlebar h2 { + margin: 0; + font-size: 1rem; +} + +.devserver-preview-modal__body { + display: flex; + flex: 1; + flex-direction: column; + min-height: 0; + overflow: hidden; +} + +.devserver-preview-modal__body .devserver-preview-container { + flex: 1; + min-height: min(60vh, calc(var(--space-2xl) * 14)); + max-height: none; +} + /* Legacy selector compatibility for static CSS tests */ .dev-server-preview-fallback { border: 1px solid color-mix(in srgb, var(--color-warning) 40%, transparent); @@ -655,7 +714,8 @@ exactly when the surrounding chrome is gone. max-width: none; } - .devserver-preview-header { + .devserver-preview-header, + .devserver-preview-modal-launcher__copy { flex-wrap: wrap; } @@ -703,6 +763,20 @@ exactly when the surrounding chrome is gone. max-height: calc(var(--space-2xl) * 3); } + .devserver-preview-modal-overlay { + align-items: stretch; + padding: var(--space-md); + } + + .devserver-preview-modal { + width: 100%; + max-height: calc(100vh - var(--space-md) * 2); + } + + .devserver-preview-modal__body .devserver-preview-container { + min-height: calc(var(--space-2xl) * 7); + } + .dev-server-config { max-height: min(48vh, calc(var(--space-2xl) * 13)); } @@ -852,7 +926,8 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu flex-direction: column; } - .devserver-preview-panel { + .devserver-preview-panel, + .devserver-preview-modal-launcher { grid-column: auto; grid-row: auto; } @@ -862,10 +937,25 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu max-width: none; } - .devserver-preview-header { + .devserver-preview-header, + .devserver-preview-modal-launcher__copy { flex-wrap: wrap; } + .devserver-preview-modal-overlay { + align-items: stretch; + padding: var(--space-md); + } + + .devserver-preview-modal { + width: min(calc(var(--space-2xl) * 20), calc(100vw - var(--space-md) * 2)); + max-height: calc(100vh - var(--space-md) * 2); + } + + .devserver-preview-modal__body .devserver-preview-container { + min-height: calc(var(--space-2xl) * 7); + } + .devserver-preview-url-badge { order: 2; flex: 1 1 100%; @@ -915,8 +1005,8 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu } .dev-server-logs, - .devserver-preview-container, - .devserver-preview-iframe { + .devserver-preview-panel .devserver-preview-container, + .devserver-preview-panel .devserver-preview-iframe { min-height: calc(var(--space-2xl) * 4 + var(--space-md)); max-height: none; } diff --git a/packages/dashboard/app/components/DevServerView.tsx b/packages/dashboard/app/components/DevServerView.tsx index e88bf63bf8..b3b88febad 100644 --- a/packages/dashboard/app/components/DevServerView.tsx +++ b/packages/dashboard/app/components/DevServerView.tsx @@ -1,13 +1,15 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import type { RefObject } from "react"; import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; -import { AlertTriangle, ExternalLink, Eye, Loader2, Monitor, Play, RefreshCw, RotateCw, ShieldAlert, Square } from "lucide-react"; +import { AlertTriangle, ExternalLink, Eye, Loader2, Monitor, Play, RefreshCw, RotateCw, ShieldAlert, Square, X } from "lucide-react"; import type { Task, TaskDetail } from "@fusion/core"; import "./DevServerView.css"; import type { DetectedDevServerCommand } from "../api"; import { useDevServer } from "../hooks/useDevServer"; import { useDevServerLogs } from "../hooks/useDevServerLogs"; import { usePreviewEmbed } from "../hooks/usePreviewEmbed"; +import { useOverlayDismiss } from "../hooks/useOverlayDismiss"; import type { ToastType } from "../hooks/useToast"; import { DevServerLogViewer } from "./DevServerLogViewer"; import { PreviewIframe } from "./PreviewIframe"; @@ -37,6 +39,85 @@ function getStatusBadgeConfig(t: TFunction<"app">): Record<"stopped" | "starting }; } + +const NARROW_RIGHT_DOCK_PREVIEW_THRESHOLD = 480; + +function isTrueMobileViewport(): boolean { + if (typeof window === "undefined" || typeof window.matchMedia !== "function") { + return false; + } + + return window.matchMedia("(max-width: 768px)").matches; +} + +function getDirectRightDockBodyHost(element: HTMLElement): HTMLElement | null { + if (element.closest(".right-dock-expand-modal__body")) { + return null; + } + + const parent = element.parentElement; + if (!parent?.classList.contains("right-dock__body")) { + return null; + } + + return parent; +} + +function readHostInlineSize(host: HTMLElement): number { + if (host.clientWidth > 0) { + return host.clientWidth; + } + + const rect = host.getBoundingClientRect(); + return rect.width; +} + +function shouldUseNarrowRightDockPreviewMode(root: HTMLElement | null): boolean { + if (!root || isTrueMobileViewport()) { + return false; + } + + const host = getDirectRightDockBodyHost(root); + if (!host) { + return false; + } + + return readHostInlineSize(host) <= NARROW_RIGHT_DOCK_PREVIEW_THRESHOLD; +} + +function useNarrowRightDockPreviewMode(rootRef: RefObject): boolean { + const [isNarrowRightDockPreviewMode, setIsNarrowRightDockPreviewMode] = useState(false); + + useEffect(() => { + const root = rootRef.current; + if (!root) { + setIsNarrowRightDockPreviewMode(false); + return; + } + + const host = getDirectRightDockBodyHost(root); + const updateMode = () => setIsNarrowRightDockPreviewMode(shouldUseNarrowRightDockPreviewMode(root)); + + updateMode(); + + if (!host || typeof ResizeObserver === "undefined") { + window.addEventListener("resize", updateMode); + return () => window.removeEventListener("resize", updateMode); + } + + const observer = new ResizeObserver(updateMode); + observer.observe(host); + window.addEventListener("resize", updateMode); + + return () => { + observer.disconnect(); + window.removeEventListener("resize", updateMode); + }; + }, [rootRef]); + + return isNarrowRightDockPreviewMode; +} + let devServerViewWasPreviouslyInactive = false; function normalizeError(error: unknown): string { @@ -142,6 +223,14 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps const effectivePreviewUrl = previewUrl; const selectedSource = session?.config?.cwd ?? null; + const rootRef = useRef(null); + const isNarrowRightDockPreviewMode = useNarrowRightDockPreviewMode(rootRef); + + /* + FNXC:DevServer 2026-06-23-00:00: + The Dev Server preview must escape into a modal when the direct right-dock host is very narrow so preview chrome does not crowd logs and configuration in the same dock column. + The 480px threshold catches the dock's compact range before preview chrome becomes unusable while preserving full-page, true mobile viewport, and expanded pop-out inline previews. + */ const [showCandidates, setShowCandidates] = useState(true); const [commandInput, setCommandInput] = useState(""); const [previewInput, setPreviewInput] = useState(""); @@ -170,6 +259,9 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps }, [executingTasks, selectedTaskId]); const [previewMode, setPreviewMode] = useState("embedded"); + const [isPreviewModalOpen, setIsPreviewModalOpen] = useState(false); + const previewModalLauncherRef = useRef(null); + const previewModalRef = useRef(null); const previewEmbedUrl = previewMode === "embedded" ? effectivePreviewUrl : null; const { @@ -271,6 +363,60 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps setPreviewInput(effectivePreviewUrl ?? ""); }, [effectivePreviewUrl]); + const closePreviewModal = useCallback(() => { + setIsPreviewModalOpen(false); + window.requestAnimationFrame(() => previewModalLauncherRef.current?.focus()); + }, []); + const previewModalOverlayDismissProps = useOverlayDismiss(closePreviewModal); + + useEffect(() => { + if (!isPreviewModalOpen) { + return; + } + + previewModalRef.current?.focus(); + + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === "Escape") { + closePreviewModal(); + return; + } + + if (event.key !== "Tab") { + return; + } + + const focusableElements = Array.from( + previewModalRef.current?.querySelectorAll( + 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])', + ) ?? [], + ).filter((element) => !element.hasAttribute("disabled") && element.getAttribute("aria-hidden") !== "true"); + + const firstElement = focusableElements[0]; + const lastElement = focusableElements.at(-1); + if (!firstElement || !lastElement) { + return; + } + + if (event.shiftKey && document.activeElement === firstElement) { + event.preventDefault(); + lastElement.focus(); + } else if (!event.shiftKey && document.activeElement === lastElement) { + event.preventDefault(); + firstElement.focus(); + } + }; + + document.addEventListener("keydown", handleKeyDown); + return () => document.removeEventListener("keydown", handleKeyDown); + }, [closePreviewModal, isPreviewModalOpen]); + + useEffect(() => { + if (!isNarrowRightDockPreviewMode && isPreviewModalOpen) { + setIsPreviewModalOpen(false); + } + }, [isNarrowRightDockPreviewMode, isPreviewModalOpen]); + const handleOpenInNewTab = useCallback(() => { if (!effectivePreviewUrl) { return; @@ -399,8 +545,136 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps const stopDisabled = status === "stopped" || actionInFlight !== null; const restartDisabled = status === "stopped" || status === "starting" || actionInFlight !== null; + const renderPreviewContent = () => ( + <> +
+
+ + {t("devserver.preview", "Preview")} +
+ + {isManualPreviewOverride ? t("devserver.manual", "Manual") : t("devserver.auto", "Auto")} + {effectivePreviewUrl ? ` · ${effectivePreviewUrl}` : t("devserver.notAvailable", " · Not available")} + +
+ + + +
+
+ +
+ {!effectivePreviewUrl && !isRunning && ( +

{t("devserver.startDevServer", "Start a dev server to see a live preview here.")}

+ )} + + {!effectivePreviewUrl && isRunning && ( +

{t("devserver.noPreviewDetected", "No preview URL detected. Start the dev server or set a manual URL to preview your app.")}

+ )} + + {effectivePreviewUrl && previewMode === "external" && ( +
+

{t("devserver.embeddedPreviewDisabled", "Embedded preview is disabled. Open your app in a separate browser tab.")}

+ +
+ )} + + {effectivePreviewUrl && previewMode === "embedded" && showFallback && isBlocked && ( +
+ {embedStatus === "error" + ?
+ )} + + {effectivePreviewUrl && previewMode === "embedded" && !showFallback && ( + + )} +
+ + ); + return ( -
+
{/* FNXC:DevServer 2026-06-22-01:00: Migrated to the shared ViewHeader for cross-view consistency. The status badge sits next to the title inside the actions slot (wrapped in .dev-server-header-title so the existing mobile flex-wrap rule still applies), and the Start/Stop/Restart controls follow in .dev-server-header-actions. ViewHeader supplies the standard view padding; the view body must not repeat the top padding. @@ -641,126 +915,75 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps
-
-
-
- - {t("devserver.preview", "Preview")} + {isNarrowRightDockPreviewMode ? ( +
+
+
+ + {t("devserver.preview", "Preview")} +
+ + {effectivePreviewUrl ? effectivePreviewUrl : t("devserver.notAvailable", "Not available")} +
- + {effectivePreviewUrl + ? t("devserver.previewModalLauncherDescription", "Open the live preview in a modal so logs and configuration stay usable in this narrow dock.") + : t("devserver.previewModalLauncherUnavailable", "Start the dev server or set a preview URL to open the preview modal.")} +

+ - - -
-
+ {t("devserver.openPreview", "Open preview")} + +
+ ) : ( +
+ {renderPreviewContent()} +
+ )} -
- {!effectivePreviewUrl && !isRunning && ( -

{t("devserver.startDevServer", "Start a dev server to see a live preview here.")}

- )} - - {!effectivePreviewUrl && isRunning && ( -

{t("devserver.noPreviewDetected", "No preview URL detected. Start the dev server or set a manual URL to preview your app.")}

- )} - - {effectivePreviewUrl && previewMode === "external" && ( -
-

{t("devserver.embeddedPreviewDisabled", "Embedded preview is disabled. Open your app in a separate browser tab.")}

+ {isNarrowRightDockPreviewMode && isPreviewModalOpen && ( +
+
+
+

{t("devserver.preview", "Preview")}

- )} - - {effectivePreviewUrl && previewMode === "embedded" && showFallback && isBlocked && ( -
- {embedStatus === "error" - ?
- + )}
); } diff --git a/packages/dashboard/app/components/__tests__/DevServerView.mobile.test.tsx b/packages/dashboard/app/components/__tests__/DevServerView.mobile.test.tsx index 09d285ec67..cf828f3b7e 100644 --- a/packages/dashboard/app/components/__tests__/DevServerView.mobile.test.tsx +++ b/packages/dashboard/app/components/__tests__/DevServerView.mobile.test.tsx @@ -51,12 +51,27 @@ describe("DevServerView mobile CSS/structure", () => { const mobileBlockMatch = css.match(/@media[^{]*\(max-width: 768px\)[^{]*\{([\s\S]*?)\n\}/g) ?? []; const mobileCss = mobileBlockMatch.join("\n"); - const headerRuleCount = (mobileCss.match(/\.devserver-preview-header\s*\{/g) ?? []).length; + const headerRuleCount = (mobileCss.match(/\.devserver-preview-header,\s*\.devserver-preview-modal-launcher__copy\s*\{/g) ?? []).length; expect(headerRuleCount).toBe(1); expect(mobileCss).toMatch(/\.devserver-preview-url-badge\s*\{[\s\S]*max-width:\s*100%/); expect(mobileCss).toMatch(/\.dev-server-header-title\s*\{[\s\S]*flex-wrap:\s*wrap/); }); + it("defines narrow right-dock launcher and modal rules without duplicating mobile media rules", () => { + const css = loadAllAppCss(); + const containerStart = css.indexOf("@container right-dock-body (max-width: 768px)"); + expect(containerStart).toBeGreaterThan(-1); + const containerCss = css.slice(containerStart); + + expect(containerCss).toMatch(/\.devserver-preview-panel,\s*\.devserver-preview-modal-launcher\s*\{[\s\S]*grid-column:\s*auto/); + expect(containerCss).toMatch(/\.devserver-preview-modal\s*\{[\s\S]*width:\s*min\(calc\(var\(--space-2xl\) \* 20\), calc\(100vw - var\(--space-md\) \* 2\)\)/); + expect(containerCss).toMatch(/\.devserver-preview-panel \.devserver-preview-container/); + expect(containerCss).not.toMatch(/\.dev-server-logs,\s*\.devserver-preview-container,\s*\.devserver-preview-iframe/); + + expect(css).toMatch(/@media[^{]*\(max-width: 768px\)/); + expect(css).toMatch(/@container right-dock-body \(max-width: 768px\)/); + }); + it("renders preview header elements and keeps URL badge outside preview actions", () => { mockUseDevServer.mockReturnValue(createDevServerHookState()); mockUseDevServerLogs.mockReturnValue({ diff --git a/packages/dashboard/app/components/__tests__/DevServerView.preview.test.tsx b/packages/dashboard/app/components/__tests__/DevServerView.preview.test.tsx index fa9f8df30f..d40f5de30f 100644 --- a/packages/dashboard/app/components/__tests__/DevServerView.preview.test.tsx +++ b/packages/dashboard/app/components/__tests__/DevServerView.preview.test.tsx @@ -44,6 +44,7 @@ vi.mock("lucide-react", () => ({ Search: () => , ShieldAlert: () => , Square: () => , + X: () => , })); function createState(overrides: Partial = {}): DevServerState { @@ -201,6 +202,153 @@ describe("DevServerView preview panel", () => { afterEach(() => { window.open = originalWindowOpen; + vi.unstubAllGlobals(); + }); + + function renderInRightDock(width: number) { + const host = document.createElement("div"); + host.className = "right-dock__body"; + Object.defineProperty(host, "clientWidth", { configurable: true, value: width }); + document.body.appendChild(host); + + return render(, { container: host }); + } + + it("activates narrow right-dock preview mode only below the dock threshold", async () => { + mockUseDevServer.mockReturnValue( + createDevServerHookState({ serverState: createState({ status: "running", previewUrl: "http://localhost:3000" }) }), + ); + + const narrow = renderInRightDock(420); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "true"); + }); + + narrow.unmount(); + document.body.innerHTML = ""; + + renderInRightDock(640); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "false"); + }); + expect(screen.queryByTestId("devserver-preview-modal-launcher")).not.toBeInTheDocument(); + expect(screen.getByTestId("devserver-preview-panel")).toBeInTheDocument(); + }); + + it("replaces the narrow right-dock inline preview with an accessible modal launcher", async () => { + mockUseDevServer.mockReturnValue( + createDevServerHookState({ serverState: createState({ status: "running", previewUrl: "http://localhost:3000" }) }), + ); + mockUseDevServerLogs.mockReturnValue(createDevServerLogsHookState({ + entries: [{ id: "log-1", timestamp: "2026-06-23T00:00:00.000Z", stream: "stdout", text: "ready" }], + total: 1, + })); + previewEmbedState = createPreviewEmbedState({ embedStatus: "embedded", isEmbedded: true }); + + renderInRightDock(420); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "true"); + }); + + expect(screen.getByTestId("dev-server-logs-panel")).toBeInTheDocument(); + expect(screen.queryByTestId("devserver-preview-panel")).not.toBeInTheDocument(); + expect(screen.queryByTitle("Dev server preview")).not.toBeInTheDocument(); + expect(screen.getByTestId("devserver-preview-modal-launcher")).toHaveTextContent("http://localhost:3000"); + expect(screen.getByTestId("devserver-preview-url-badge")).toHaveTextContent("http://localhost:3000"); + + fireEvent.click(screen.getByTestId("devserver-preview-modal-open")); + + const modal = await screen.findByTestId("devserver-preview-modal"); + expect(modal).toHaveAttribute("role", "dialog"); + expect(modal).toHaveAttribute("aria-modal", "true"); + expect(screen.getByTitle("Dev server preview")).toBeInTheDocument(); + expect(screen.getByTestId("devserver-preview-open-tab")).toBeInTheDocument(); + expect(screen.getByTestId("devserver-preview-refresh")).toBeInTheDocument(); + + fireEvent.keyDown(document, { key: "Escape" }); + + await waitFor(() => { + expect(screen.queryByTestId("devserver-preview-modal")).not.toBeInTheDocument(); + }); + }); + + it("keeps preview modes and fallback actions inside the narrow dock modal", async () => { + const retry = vi.fn(); + mockUseDevServer.mockReturnValue( + createDevServerHookState({ serverState: createState({ status: "running", previewUrl: "http://localhost:3000" }) }), + ); + previewEmbedState = createPreviewEmbedState({ embedStatus: "embedded", isEmbedded: true }); + + const { rerender } = renderInRightDock(420); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "true"); + }); + + fireEvent.click(screen.getByTestId("devserver-preview-modal-open")); + + previewEmbedState = createPreviewEmbedState({ + embedStatus: "blocked", + isBlocked: true, + embedContext: "The server may block iframe embedding...", + retry, + }); + rerender(); + + await waitFor(() => { + expect(screen.getByTestId("devserver-preview-fallback")).toBeInTheDocument(); + }); + expect(screen.getByText("Preview blocked")).toBeInTheDocument(); + fireEvent.click(screen.getByTestId("devserver-preview-fallback-retry")); + expect(retry).toHaveBeenCalledTimes(1); + + previewEmbedState = createPreviewEmbedState({ embedStatus: "embedded", isEmbedded: true }); + rerender(); + fireEvent.click(screen.getByTestId("devserver-preview-mode-toggle")); + + expect(screen.getByTestId("devserver-preview-external-only")).toBeInTheDocument(); + fireEvent.click(screen.getByTestId("devserver-preview-external-open-tab")); + expect(window.open).toHaveBeenCalledWith("http://localhost:3000", "_blank", "noopener,noreferrer"); + }); + + it("keeps inline preview mode for true mobile viewport and expanded right-dock hosts", async () => { + vi.stubGlobal("matchMedia", vi.fn().mockImplementation((query: string) => ({ + matches: query === "(max-width: 768px)", + media: query, + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn(), + }))); + mockUseDevServer.mockReturnValue( + createDevServerHookState({ serverState: createState({ status: "running", previewUrl: "http://localhost:3000" }) }), + ); + + const mobile = renderInRightDock(420); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "false"); + }); + + mobile.unmount(); + document.body.innerHTML = ""; + vi.unstubAllGlobals(); + + const expandedHost = document.createElement("div"); + expandedHost.className = "right-dock-expand-modal__body"; + Object.defineProperty(expandedHost, "clientWidth", { configurable: true, value: 420 }); + document.body.appendChild(expandedHost); + + render(, { container: expandedHost }); + + await waitFor(() => { + expect(screen.getByTestId("dev-server-view")).toHaveAttribute("data-narrow-right-dock-preview", "false"); + }); }); it("shows start-empty state when server is not configured", () => { From be0cab1d43b4f68fa647393b31ea1b8e9052cec8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 23 Jun 2026 16:47:08 -0700 Subject: [PATCH 17/17] fix(dashboard): raise model dropdown z-index above floating windows Raise CustomModelDropdown portal z-index from 1200 to 11000 so model selection popups render above the shared floating-window stack (10100+) instead of being obscured by popped-out ChatView and other floating modals. --- packages/dashboard/app/components/CustomModelDropdown.css | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dashboard/app/components/CustomModelDropdown.css b/packages/dashboard/app/components/CustomModelDropdown.css index e943c7f93d..933c7d29f6 100644 --- a/packages/dashboard/app/components/CustomModelDropdown.css +++ b/packages/dashboard/app/components/CustomModelDropdown.css @@ -67,8 +67,8 @@ border: 1px solid var(--border); border-radius: var(--radius); box-shadow: var(--shadow); - /* Must sit above floating dashboard panels. */ - z-index: 1200; + /* Must sit above floating dashboard panels and the shared floating-window stack (10100+). */ + z-index: 11000; max-height: 320px; display: flex; flex-direction: column;