Skip to content

Commit 3bb5b23

Browse files
yohamta0yohamta0
authored
fix(ui): keep spec editor in place while preview updates (#2947)
1 parent 5d43e0d commit 3bb5b23

7 files changed

Lines changed: 514 additions & 81 deletions

File tree

‎ui/e2e/dag-crud.spec.ts‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,85 @@ steps:
103103
await expect(warning).toHaveCount(0);
104104
});
105105

106+
// Live validation redraws the graph, step table and errors above the
107+
// editor. None of that may move the editor while the user types.
108+
test('keeps the spec editor in place while the preview updates', async ({ page, request }) => {
109+
const stack = await loadStack();
110+
const token = await loginViaAPI(
111+
request,
112+
stack.auth.adminUsername,
113+
stack.auth.adminPassword
114+
);
115+
const dagName = uniqueName('e2e-spec-anchor');
116+
const definition = `name: ${dagName}
117+
steps:
118+
- name: first
119+
run: echo first
120+
`;
121+
const fileName = await writeLocalDAG(dagName, definition);
122+
await waitForDAGAvailable(request, token, fileName);
123+
await page.goto(`/dags/${encodeURIComponent(fileName)}/spec`);
124+
125+
const graph = page.locator('.mermaid svg');
126+
const monaco = page.locator('.monaco-editor').first();
127+
await expect(graph.getByText('first', { exact: true })).toBeVisible();
128+
await expect(monaco).toBeVisible();
129+
// Leave the bottom of the preview in view, where browser scroll
130+
// anchoring alone would pin the preview rather than the editor.
131+
await page
132+
.getByRole('heading', { name: 'YAML', exact: true })
133+
.evaluate((heading) => {
134+
heading.scrollIntoView({ block: 'start' });
135+
let scroller = heading.parentElement;
136+
while (scroller && getComputedStyle(scroller).overflowY !== 'auto') {
137+
scroller = scroller.parentElement;
138+
}
139+
scroller?.scrollBy(0, -150);
140+
});
141+
const editorTop = async () => (await monaco.boundingBox())?.y ?? NaN;
142+
const initialTop = await editorTop();
143+
// Scroll positions snap to whole pixels while preview heights do not.
144+
const expectEditorInPlace = async () =>
145+
expect(Math.abs((await editorTop()) - initialTop)).toBeLessThan(2);
146+
147+
// Single-line flow YAML sidesteps Monaco's auto-indent on typed newlines.
148+
// Brackets Monaco auto-closes and does not overtype end up after the
149+
// cursor, so the rest of the line is dropped. Select-all right after
150+
// focusing is occasionally lost, so the replacement is retried until the
151+
// buffer is that single line.
152+
const replaceSpec = async (spec: string) => {
153+
await expect(async () => {
154+
await page.keyboard.press('ControlOrMeta+A');
155+
await page.keyboard.insertText(spec);
156+
await page.keyboard.press('Shift+End');
157+
await page.keyboard.press('Delete');
158+
await expect(monaco.locator('.view-line')).toHaveCount(1, {
159+
timeout: 500,
160+
});
161+
}).toPass();
162+
};
163+
const step = (name: string, extra = '') =>
164+
`{name: ${name}, run: echo ${name}${extra}}`;
165+
await page.locator('.monaco-editor textarea').first().focus();
166+
await expect(monaco).toHaveClass(/\bfocused\b/);
167+
168+
await replaceSpec('steps: []');
169+
await expect(page.getByText('No steps to render')).toBeVisible();
170+
await expectEditorInPlace();
171+
172+
await replaceSpec(`steps: [${step('first')}, ${step('second')}, ${step('third')}]`);
173+
await expect(page.getByText('Valid', { exact: true })).toBeVisible();
174+
await expect(graph.getByText('third', { exact: true })).toBeVisible();
175+
await expectEditorInPlace();
176+
177+
await replaceSpec(
178+
`steps: [${step('first')}, ${step('second')}, ` +
179+
`${step('third', ', depends: [missing]')}]`
180+
);
181+
await expect(page.getByText(/^\d+ issues?$/)).toBeVisible();
182+
await expectEditorInPlace();
183+
});
184+
106185
test('renames a DAG from the UI', async ({ page, request }) => {
107186
const stack = await loadStack();
108187
const token = await loginViaAPI(

‎ui/src/components/ui/__tests__/mermaid.test.tsx‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ describe('Mermaid', () => {
5151

5252
afterEach(() => {
5353
vi.useRealTimers();
54+
vi.restoreAllMocks();
5455
});
5556

5657
it('ignores stale render results after a newer definition renders', async () => {
@@ -111,6 +112,31 @@ describe('Mermaid', () => {
111112
expect(screen.queryByText('Fallback graph')).not.toBeInTheDocument();
112113
});
113114

115+
// Re-rendering removes the old SVG before the new one is ready. Content
116+
// below the graph must not collapse and jump in the meantime. jsdom has no
117+
// layout engine, so the rendered height is stubbed.
118+
it('keeps its height while a new definition renders', async () => {
119+
vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockReturnValue(240);
120+
const { container, rerender } = render(
121+
<Mermaid def="graph TD; A-->B;" scale={1} />
122+
);
123+
await waitFor(() => expect(pendingRenders).toHaveLength(1));
124+
await act(async () => {
125+
pendingRenderAt(0).resolve({ svg: '<svg data-def="first"></svg>' });
126+
});
127+
const graph = container.querySelector<HTMLElement>('.mermaid');
128+
const restingMinHeight = graph?.style.minHeight;
129+
130+
rerender(<Mermaid def="graph TD; C-->D;" scale={1} />);
131+
await waitFor(() => expect(pendingRenders).toHaveLength(2));
132+
expect(graph?.style.minHeight).toBe('240px');
133+
134+
await act(async () => {
135+
pendingRenderAt(1).resolve({ svg: '<svg data-def="second"></svg>' });
136+
});
137+
expect(graph?.style.minHeight).toBe(restingMinHeight);
138+
});
139+
114140
it('resolves Mermaid 11.15 prefixed node ids before firing graph callbacks', async () => {
115141
vi.useFakeTimers();
116142
const onClick = vi.fn();

‎ui/src/components/ui/mermaid.tsx‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,15 @@ function Mermaid({
301301
return;
302302
}
303303

304+
// Hold the current height while the old SVG is gone and the new one is
305+
// not ready, so content below the graph does not collapse and jump.
306+
const releaseHeight = () => {
307+
if (mermaidRef.current) {
308+
mermaidRef.current.style.minHeight = String(mStyle.minHeight ?? '');
309+
}
310+
};
311+
mermaidRef.current.style.minHeight = `${mermaidRef.current.offsetHeight}px`;
312+
304313
try {
305314
setRenderError(null);
306315
// Reinitialize Mermaid to pick up current theme
@@ -320,6 +329,7 @@ function Mermaid({
320329
}
321330

322331
mermaidRef.current.innerHTML = svg;
332+
releaseHeight();
323333
onRender?.(mermaidRef.current);
324334
applyNodeInteractionStyles(
325335
mermaidRef.current,
@@ -374,6 +384,7 @@ function Mermaid({
374384
}
375385
console.error('Mermaid render error:', error);
376386
setRenderError(String(error));
387+
releaseHeight();
377388
if (mermaidRef.current) {
378389
mermaidRef.current.innerHTML = '';
379390
}

‎ui/src/features/dags/components/dag-editor/DAGSpec.tsx‎

Lines changed: 88 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ import DAGEditorWithDocs from './DAGEditorWithDocs';
6464
import { parseValidationMarkers } from './validationMarkers';
6565
import { AgentSpecOverview } from './AgentSpecOverview';
6666
import ExternalChangeDialog from './ExternalChangeDialog';
67+
import { useEditorScrollAnchor } from './useEditorScrollAnchor';
6768
import { I18nText } from '@/i18n/I18nText';
6869
import { I18nProps } from '@/i18n/I18nProps';
6970
import { useI18n } from '@/i18n/I18nProvider';
@@ -121,6 +122,10 @@ function DAGSpec({ fileName, localDags, editorHints }: Props) {
121122
// Reference to the main container div
122123
const containerRef = React.useRef<HTMLDivElement>(null);
123124

125+
// Keeps the YAML editor still while the live preview above it resizes
126+
const { anchorRef: editorSectionRef, contentRef: previewRef } =
127+
useEditorScrollAnchor();
128+
124129
// Reference to save function and refresh callback for keyboard shortcut
125130
const saveHandlerRef = React.useRef<(() => Promise<void>) | null>(null);
126131
const refreshCallbackRef = React.useRef<(() => void) | null>(null);
@@ -206,6 +211,8 @@ function DAGSpec({ fileName, localDags, editorHints }: Props) {
206211

207212
// Live server-side validation of the edited buffer. Cleared whenever the
208213
// buffer stops being dirty (save or discard), which also clears the markers.
214+
// Kept while the next check is pending so the preview above the editor does
215+
// not flip back to the saved spec on every keystroke.
209216
const [liveValidation, setLiveValidation] = React.useState<{
210217
errors: string[];
211218
warnings: string[];
@@ -224,7 +231,6 @@ function DAGSpec({ fileName, localDags, editorHints }: Props) {
224231
}
225232

226233
const seq = ++validateSeqRef.current;
227-
setLiveValidation(null);
228234
setIsValidating(true);
229235
const timer = window.setTimeout(() => {
230236
void client
@@ -760,90 +766,95 @@ function DAGSpec({ fileName, localDags, editorHints }: Props) {
760766
className="flex min-h-0 flex-1 flex-col space-y-6 pb-8"
761767
ref={containerRef}
762768
>
763-
{warnings.length > 0 && (
764-
<div
765-
role="status"
766-
className="rounded-md border border-amber-500/30 bg-amber-500/10 p-3 text-sm text-amber-800 dark:text-amber-200"
767-
>
768-
<div className="mb-2 flex items-center gap-2 font-medium">
769-
<AlertTriangle className="h-4 w-4" aria-hidden="true" />
770-
<I18nText text={'Warnings'} />
769+
<div ref={previewRef} className="flex-shrink-0 space-y-6">
770+
{warnings.length > 0 && (
771+
<div
772+
role="status"
773+
className="rounded-md border border-amber-500/30 bg-amber-500/10 p-3 text-sm text-amber-800 dark:text-amber-200"
774+
>
775+
<div className="mb-2 flex items-center gap-2 font-medium">
776+
<AlertTriangle className="h-4 w-4" aria-hidden="true" />
777+
<I18nText text={'Warnings'} />
778+
</div>
779+
<ul className="list-disc space-y-1 pl-5">
780+
{warnings.map((warning) => (
781+
<li
782+
key={warning}
783+
className="whitespace-normal break-words"
784+
>
785+
{warning}
786+
</li>
787+
))}
788+
</ul>
771789
</div>
772-
<ul className="list-disc space-y-1 pl-5">
773-
{warnings.map((warning) => (
774-
<li
775-
key={warning}
776-
className="whitespace-normal break-words"
777-
>
778-
{warning}
779-
</li>
780-
))}
781-
</ul>
782-
</div>
783-
)}
784-
{hasLocalDags && (
785-
<div className="flex-shrink-0">
786-
<div className="overflow-x-auto -mx-2 px-2 scrollbar-thin scrollbar-thumb-gray-300">
787-
<Tabs className="w-max min-w-full">
788-
<Tab
789-
isActive={activeTab === 'parent'}
790-
onClick={() => handleActiveTabChange('parent')}
791-
className="cursor-pointer whitespace-nowrap"
792-
>
793-
{data?.dag?.name} <I18nText text={'(Parent)'} />
794-
</Tab>
795-
{localDags?.map(
796-
(localDag: components['schemas']['LocalDag']) => (
797-
<Tab
798-
key={localDag.name}
799-
isActive={activeTab === localDag.name}
800-
onClick={() =>
801-
handleActiveTabChange(localDag.name)
802-
}
803-
className="cursor-pointer whitespace-nowrap"
804-
>
805-
{localDag.name}
806-
</Tab>
807-
)
808-
)}
809-
</Tabs>
790+
)}
791+
{hasLocalDags && (
792+
<div className="flex-shrink-0">
793+
<div className="overflow-x-auto -mx-2 px-2 scrollbar-thin scrollbar-thumb-gray-300">
794+
<Tabs className="w-max min-w-full">
795+
<Tab
796+
isActive={activeTab === 'parent'}
797+
onClick={() => handleActiveTabChange('parent')}
798+
className="cursor-pointer whitespace-nowrap"
799+
>
800+
{data?.dag?.name} <I18nText text={'(Parent)'} />
801+
</Tab>
802+
{localDags?.map(
803+
(localDag: components['schemas']['LocalDag']) => (
804+
<Tab
805+
key={localDag.name}
806+
isActive={activeTab === localDag.name}
807+
onClick={() =>
808+
handleActiveTabChange(localDag.name)
809+
}
810+
className="cursor-pointer whitespace-nowrap"
811+
>
812+
{localDag.name}
813+
</Tab>
814+
)
815+
)}
816+
</Tabs>
817+
</div>
810818
</div>
811-
</div>
812-
)}
813-
814-
{(() => {
815-
if (activeTab === 'parent') {
816-
// While the buffer is dirty, preview the live validation
817-
// result instead of the saved spec.
818-
const previewDag = liveValidation?.dag ?? data?.dag;
819-
const previewErrors = liveValidation
820-
? liveValidation.errors
821-
: data?.errors;
819+
)}
820+
821+
{(() => {
822+
if (activeTab === 'parent') {
823+
// While the buffer is dirty, preview the live validation
824+
// result instead of the saved spec.
825+
const previewDag = liveValidation?.dag ?? data?.dag;
826+
const previewErrors = liveValidation
827+
? liveValidation.errors
828+
: data?.errors;
829+
return (
830+
previewDag && (
831+
<div className="flex-shrink-0">
832+
{renderDAGContent(previewDag, previewErrors)}
833+
</div>
834+
)
835+
);
836+
}
837+
const selectedLocalDag = localDags?.find(
838+
(ld: components['schemas']['LocalDag']) =>
839+
ld.name === activeTab
840+
);
822841
return (
823-
previewDag && (
842+
selectedLocalDag?.dag && (
824843
<div className="flex-shrink-0">
825-
{renderDAGContent(previewDag, previewErrors)}
844+
{renderDAGContent(
845+
selectedLocalDag.dag,
846+
selectedLocalDag.errors
847+
)}
826848
</div>
827849
)
828850
);
829-
}
830-
const selectedLocalDag = localDags?.find(
831-
(ld: components['schemas']['LocalDag']) =>
832-
ld.name === activeTab
833-
);
834-
return (
835-
selectedLocalDag?.dag && (
836-
<div className="flex-shrink-0">
837-
{renderDAGContent(
838-
selectedLocalDag.dag,
839-
selectedLocalDag.errors
840-
)}
841-
</div>
842-
)
843-
);
844-
})()}
851+
})()}
852+
</div>
845853

846-
<section className="flex-shrink-0 space-y-3">
854+
<section
855+
ref={editorSectionRef}
856+
className="flex-shrink-0 space-y-3"
857+
>
847858
<h2 className="text-lg font-semibold text-foreground">
848859
<I18nText text={'YAML'} />
849860
</h2>

0 commit comments

Comments
 (0)