refactor(markdown-editor): complete the field handle in raw mode, delete the settings-prompt bypass

Third-review MEDIUM-1: the field's imperative handle implemented cycleToVariable/
insertAtCursor only in rich mode, so settings-prompt reached around the component
with document.querySelector('#{formId} textarea') and hand-rolled a cycle algorithm
that was a structural duplicate of the rich one (drift hazard) plus formId plumbing,
a synthetic field object, a setTimeout caret restore, and a 35-line caretOffsetTop
mirror — ~80 lines of consumer bypass. This completes the already-shipped handle
contract (the same principle as making focus() work in both modes): the raw branch
now cycles/inserts over its textarea, both methods are required (drop the optional
`?`), and the cyclic "next occurrence" pick is one shared pure helper
(nextVariableRange) used by both surfaces. settings-prompt's handleVariableClick
collapses to mode-agnostic editorRef.current.cycleToVariable/insertAtCursor.

Also from the third review: LOW-1 the raw cn() argument order now puts the byte-exact
font-mono/no-resize classes last so a consumer className genuinely can't override them
(the comment claimed a contract the code didn't enforce); LOW-3 the handle contract is
JSDoc; LOW-5 the Suspense fallback forwards id/aria/disabled; LOW-6 the loading skeleton
and the editor's own placeholder share one wrapper class (extracted to a chunk-light
module) so first rich mount no longer flashes a different box; INFO-1 drop the unused
MarkdownEditorHandle barrel export; and a field-level unit test pins the mode-aware handle.

Live-verified on docker: raw + rich variable cycle work through the handle (querySelector
gone), console clean; 872 vitest green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
2026-07-05 13:21:35 +07:00
co-authored by Claude Opus 4.8
parent 943b2aab01
commit 2e9eb96f75
8 changed files with 302 additions and 183 deletions
@@ -1,4 +1,3 @@
export type { MarkdownEditorHandle } from './markdown-editor';
// Deliberately does NOT re-export the heavy MarkdownEditor value — that would statically pull the tiptap chunk
// into any route importing a light util from here. Consume the mode-switching MarkdownEditorField instead (it
// owns the lazy() boundary, so importing it is chunk-free until rich mode renders).
@@ -0,0 +1,106 @@
import { render, waitFor } from '@testing-library/react';
import { createRef } from 'react';
import { beforeAll, describe, expect, it, vi } from 'vitest';
import { MarkdownEditorField, type MarkdownEditorFieldHandle } from './markdown-editor-field';
import { setupEditorJsdom } from './markdown-editor-test-setup';
import { nextVariableRange } from './markdown-editor-variable-syntax';
beforeAll(setupEditorJsdom);
describe('nextVariableRange', () => {
const ranges = [
{ end: 5, start: 0 },
{ end: 15, start: 10 },
];
it('returns undefined when there are no occurrences', () => {
expect(nextVariableRange([], 0, 0)).toBeUndefined();
});
it('advances from the selected occurrence and wraps past the last', () => {
expect(nextVariableRange(ranges, 0, 5)).toEqual({ end: 15, start: 10 });
expect(nextVariableRange(ranges, 10, 15)).toEqual({ end: 5, start: 0 });
});
it('jumps to the first occurrence at/after the caret when none is selected', () => {
expect(nextVariableRange(ranges, 6, 6)).toEqual({ end: 15, start: 10 });
expect(nextVariableRange(ranges, 99, 99)).toEqual({ end: 5, start: 0 });
});
});
describe('MarkdownEditorField raw-mode handle', () => {
it('focuses, cycles occurrences, and inserts through the textarea', () => {
const onChange = vi.fn();
const ref = createRef<MarkdownEditorFieldHandle>();
const { container } = render(
<MarkdownEditorField
mode="raw"
onChange={onChange}
ref={ref}
value={'a {{.Foo}} b {{.Foo}} c'}
/>,
);
const textarea = container.querySelector('textarea')!;
ref.current!.focus();
expect(document.activeElement).toBe(textarea);
expect(ref.current!.cycleToVariable('Nope')).toBe(false);
expect(ref.current!.cycleToVariable('Foo')).toBe(true);
expect(textarea.value.slice(textarea.selectionStart, textarea.selectionEnd)).toBe('{{.Foo}}');
textarea.setSelectionRange(0, 0);
ref.current!.insertAtCursor('{{.Bar}}');
expect(onChange).toHaveBeenCalledWith(expect.stringContaining('{{.Bar}}'));
});
});
describe('MarkdownEditorField rich-mode handle', () => {
it('lazy-mounts the editor and delegates handle methods to it', async () => {
const ref = createRef<MarkdownEditorFieldHandle>();
const { container } = render(
<MarkdownEditorField
mode="rich"
onChange={vi.fn()}
ref={ref}
value={'x {{.Foo}} y'}
/>,
);
await waitFor(() => expect(container.querySelector('.ProseMirror')?.textContent).toContain('Foo'));
expect(ref.current?.cycleToVariable('Foo')).toBe(true);
expect(ref.current?.cycleToVariable('Nope')).toBe(false);
});
});
describe('MarkdownEditorField handle contract', () => {
it('exposes focus, cycleToVariable, and insertAtCursor in both modes', async () => {
const rawRef = createRef<MarkdownEditorFieldHandle>();
render(
<MarkdownEditorField
mode="raw"
onChange={vi.fn()}
ref={rawRef}
value=""
/>,
);
const richRef = createRef<MarkdownEditorFieldHandle>();
const { container } = render(
<MarkdownEditorField
mode="rich"
onChange={vi.fn()}
ref={richRef}
value=""
/>,
);
await waitFor(() => expect(container.querySelector('.ProseMirror')).not.toBeNull());
for (const current of [rawRef.current, richRef.current]) {
expect(typeof current?.focus).toBe('function');
expect(typeof current?.cycleToVariable).toBe('function');
expect(typeof current?.insertAtCursor).toBe('function');
}
});
});
@@ -11,22 +11,25 @@ import { cn } from '@/lib/utils';
import type { MarkdownEditorHandle } from './markdown-editor';
import type { EditorViewMode } from './markdown-editor-view-mode';
// Static-importing MarkdownEditor would merge the tiptap chunk into every route that pulls a util from the
// barrel; the lazy() boundary is what lets the barrel re-export this component eagerly.
import { MARKDOWN_EDITOR_WRAPPER_CLASS } from './markdown-editor-styles';
import { cycleTextareaToVariable, insertTextareaText } from './markdown-editor-textarea';
// Static-importing MarkdownEditor would merge the tiptap chunk into every route that pulls a util from the barrel.
const MarkdownEditor = lazy(() => import('./markdown-editor').then((module) => ({ default: module.MarkdownEditor })));
// The imperative handle a consumer gets via `ref`. `focus()` is honored in BOTH modes (so RHF's
// focus-on-validation-error reaches the field whether it renders a textarea or the rich editor). The
// variable-panel methods exist only in rich mode — raw has no ProseMirror doc to cycle — so they are optional.
/**
* Imperative handle exposed via `ref`. Every method works in BOTH modes — the raw textarea and the rich editor
* each implement them — so a consumer drives the field the same way whether it renders raw source or rich.
*/
export interface MarkdownEditorFieldHandle {
cycleToVariable?: (variable: string) => boolean;
/** Select the next occurrence of `variable` and scroll it into view; `false` when it isn't used. */
cycleToVariable: (variable: string) => boolean;
focus: () => void;
insertAtCursor?: (text: string) => void;
/** Insert `text` at the caret, replacing any selection. */
insertAtCursor: (text: string) => void;
}
interface MarkdownEditorFieldProps extends Pick<AriaAttributes, 'aria-describedby' | 'aria-invalid'> {
// Sizes the field's outer box in both modes — the rich editor wrapper and the raw <textarea> take the
// same flex/min-height layout. The byte-exact font-mono / no-resize raw config is baked in, not overridable.
className?: string;
disabled?: boolean;
id?: string;
@@ -58,13 +61,22 @@ export function MarkdownEditorField({
ref,
() =>
mode === 'raw'
? { focus: () => rawRef.current?.focus() }
? {
cycleToVariable: (variable) =>
rawRef.current ? cycleTextareaToVariable(rawRef.current.textarea, variable) : false,
focus: () => rawRef.current?.focus(),
insertAtCursor: (text) => {
if (rawRef.current) {
insertTextareaText(rawRef.current.textarea, text, onChange);
}
},
}
: {
cycleToVariable: (variable) => richRef.current?.cycleToVariable(variable) ?? false,
focus: () => richRef.current?.focus(),
insertAtCursor: (text) => richRef.current?.insertAtCursor(text),
},
[mode],
[mode, onChange],
);
if (mode === 'raw') {
@@ -73,7 +85,8 @@ export function MarkdownEditorField({
aria-describedby={ariaDescribedby}
aria-invalid={ariaInvalid}
autoSize={false}
className={cn('resize-none font-mono text-sm', className)}
// Raw config is applied LAST so a consumer `className` can't override the byte-exact source styling.
className={cn(className, 'resize-none font-mono text-sm')}
disabled={disabled}
id={id}
onBlur={onBlur}
@@ -90,7 +103,15 @@ export function MarkdownEditorField({
fallback={
<div
aria-busy="true"
className={cn('flex items-center justify-center rounded-md border', className)}
aria-describedby={ariaDescribedby}
aria-invalid={ariaInvalid}
className={cn(
MARKDOWN_EDITOR_WRAPPER_CLASS,
'items-center justify-center',
disabled && 'pointer-events-none opacity-60',
className,
)}
id={id}
>
<Loader2 className="text-muted-foreground size-5 animate-spin" />
</div>
@@ -0,0 +1,4 @@
// Shell classes for the editor box. Kept in this tiny leaf module (no tiptap import) so MarkdownEditorField's
// lazy-load fallback can wear the same box as the mounted editor without pulling the tiptap chunk eagerly.
export const MARKDOWN_EDITOR_WRAPPER_CLASS =
'border-input dark:bg-input/30 group/markdown-editor flex w-full flex-col overflow-hidden rounded-md border shadow-2xs outline-hidden transition-[color,box-shadow]';
@@ -0,0 +1,77 @@
import { findVariableUseRanges, nextVariableRange } from './markdown-editor-variable-syntax';
/** Select the next occurrence of `variable` in the textarea and scroll it to the middle. Returns false if none. */
export function cycleTextareaToVariable(textarea: HTMLTextAreaElement, variable: string): boolean {
const ranges = findVariableUseRanges(textarea.value, variable).map(({ index, length }) => ({
end: index + length,
start: index,
}));
const target = nextVariableRange(ranges, textarea.selectionStart, textarea.selectionEnd);
if (!target) {
return false;
}
textarea.focus();
textarea.setSelectionRange(target.start, target.end);
textarea.scrollTop = Math.max(0, caretOffsetTop(textarea, target.start) - textarea.clientHeight / 2);
return true;
}
/** Splice `text` over the textarea selection, emit through `onChange`, and restore the caret after the value round-trips. */
export function insertTextareaText(
textarea: HTMLTextAreaElement,
text: string,
onChange: (value: string) => void,
): void {
const start = Math.max(0, textarea.selectionStart);
const end = Math.max(0, textarea.selectionEnd);
const { value } = textarea;
onChange(value.slice(0, start) + text + value.slice(end));
// The controlled re-render clears the selection; restore the caret once React has flushed the new value.
setTimeout(() => {
textarea.focus({ preventScroll: true });
textarea.setSelectionRange(start + text.length, start + text.length);
}, 0);
}
// Pixel offset of `position` from the textarea content top, measured via a hidden mirror so soft-wrapped
// lines count — a logical-line count undershoots the scroll badly for wrapped templates.
function caretOffsetTop(textarea: HTMLTextAreaElement, position: number): number {
const cs = getComputedStyle(textarea);
const mirror = document.createElement('div');
mirror.style.fontFamily = cs.fontFamily;
mirror.style.fontSize = cs.fontSize;
mirror.style.fontWeight = cs.fontWeight;
mirror.style.fontStyle = cs.fontStyle;
mirror.style.lineHeight = cs.lineHeight;
mirror.style.letterSpacing = cs.letterSpacing;
mirror.style.wordSpacing = cs.wordSpacing;
mirror.style.paddingTop = cs.paddingTop;
mirror.style.paddingRight = cs.paddingRight;
mirror.style.paddingBottom = cs.paddingBottom;
mirror.style.paddingLeft = cs.paddingLeft;
mirror.style.width = `${textarea.clientWidth}px`;
mirror.style.boxSizing = 'border-box';
mirror.style.whiteSpace = 'pre-wrap';
mirror.style.overflowWrap = 'break-word';
mirror.style.position = 'absolute';
mirror.style.visibility = 'hidden';
mirror.style.top = '-9999px';
mirror.style.left = '-9999px';
mirror.textContent = textarea.value.slice(0, position);
const marker = document.createElement('span');
marker.textContent = textarea.value.charAt(position) || '.';
mirror.appendChild(marker);
document.body.appendChild(mirror);
const offset = marker.offsetTop;
mirror.remove();
return offset;
}
@@ -26,3 +26,23 @@ export const findVariableUseRanges = (value: string, variable: string): { index:
return ranges;
};
export interface VariableRange {
end: number;
start: number;
}
// Shared by BOTH variable panels (rich ProseMirror + raw textarea) so their cycling can't drift: from the
// currently-selected occurrence go to the next (wrapping); if the caret isn't on one, jump to the first
// occurrence at/after it, else wrap to the first. `undefined` when there are none.
export function nextVariableRange(
ranges: VariableRange[],
selectionStart: number,
selectionEnd: number,
): undefined | VariableRange {
const current = ranges.findIndex((range) => range.start === selectionStart && range.end === selectionEnd);
return current >= 0
? ranges[(current + 1) % ranges.length]
: (ranges.find((range) => range.start >= selectionStart) ?? ranges[0]);
}
@@ -4,13 +4,16 @@ import type { AriaAttributes, Ref } from 'react';
import { history } from '@tiptap/pm/history';
import { EditorState, TextSelection } from '@tiptap/pm/state';
import { EditorContent, useEditor } from '@tiptap/react';
import { Loader2 } from 'lucide-react';
import { useEffect, useImperativeHandle, useMemo, useRef, useState } from 'react';
import { cn } from '@/lib/utils';
import { createMarkdownExtensions } from './markdown-editor-extensions';
import { MARKDOWN_EDITOR_WRAPPER_CLASS as WRAPPER_CLASS } from './markdown-editor-styles';
import { MarkdownEditorToolbar } from './markdown-editor-toolbar';
import { findVariableOccurrences } from './markdown-editor-variable-highlight';
import { nextVariableRange } from './markdown-editor-variable-syntax';
export interface MarkdownEditorHandle {
cycleToVariable: (variable: string) => boolean;
@@ -66,9 +69,6 @@ export const resetUndoHistory = (editor: Editor): void => {
view.dispatch(view.state.tr.setMeta('addToHistory', false));
};
const WRAPPER_CLASS =
'border-input dark:bg-input/30 group/markdown-editor flex w-full flex-col overflow-hidden rounded-md border shadow-2xs outline-hidden transition-[color,box-shadow]';
function MarkdownEditor({
'aria-describedby': ariaDescribedby,
'aria-invalid': ariaInvalid,
@@ -140,18 +140,12 @@ function MarkdownEditor({
}
const { state, view } = editor;
const hits = findVariableOccurrences(state.doc, variable);
if (hits.length === 0) {
return false;
}
const hits = findVariableOccurrences(state.doc, variable).map(({ from, to }) => ({
end: to,
start: from,
}));
const { from, to } = state.selection;
const currentIndex = hits.findIndex((hit) => hit.from === from && hit.to === to);
const target =
currentIndex >= 0
? hits[(currentIndex + 1) % hits.length]
: (hits.find((hit) => hit.from >= from) ?? hits[0]);
const target = nextVariableRange(hits, from, to);
if (!target) {
return false;
@@ -160,7 +154,7 @@ function MarkdownEditor({
// Focus first: ProseMirror won't scrollIntoView an unfocused editor (first post-load click would no-op).
view.focus();
view.dispatch(
state.tr.setSelection(TextSelection.create(state.doc, target.from, target.to)).scrollIntoView(),
state.tr.setSelection(TextSelection.create(state.doc, target.start, target.end)).scrollIntoView(),
);
return true;
@@ -258,9 +252,16 @@ function MarkdownEditor({
return (
<div
aria-busy="true"
className={cn(WRAPPER_CLASS, disabled && 'pointer-events-none opacity-60', className)}
className={cn(
WRAPPER_CLASS,
'items-center justify-center',
disabled && 'pointer-events-none opacity-60',
className,
)}
data-slot="markdown-editor"
/>
>
<Loader2 className="text-muted-foreground size-5 animate-spin" />
</div>
);
}
+43 -152
View File
@@ -40,7 +40,6 @@ import type {
import {
type EditorViewMode,
EditorViewModeToggle,
findVariableUseRanges,
MarkdownEditorField,
type MarkdownEditorFieldHandle,
VARIABLE_RE,
@@ -203,6 +202,39 @@ const diffStyles = {
},
} satisfies ComponentProps<typeof ReactDiffViewer>['styles'];
interface DiffContentProps {
control: Control<HumanFormData> | Control<SystemFormData>;
oldValue: string;
styles: ComponentProps<typeof ReactDiffViewer>['styles'];
}
interface VariablesPanelContainerProps {
control: Control<HumanFormData> | Control<SystemFormData>;
onVariableClick: (variable: string) => void;
variables: string[];
}
interface VariablesProps {
currentTemplate: string;
onVariableClick: (variable: string) => void;
variables: string[];
}
// Don't hoist this useWatch to the parent — it would re-subscribe the whole page per keystroke.
function DiffContent({ control, oldValue, styles }: DiffContentProps) {
const newValue = useWatch({ control, name: 'template' });
return (
<ReactDiffViewer
newValue={newValue}
oldValue={oldValue}
splitView
styles={styles}
useDarkTheme
/>
);
}
function FormMarkdownItem<T extends FieldValues>({
control,
disabled,
@@ -239,77 +271,6 @@ function FormMarkdownItem<T extends FieldValues>({
);
}
// Pixel offset of `position` from the textarea content top, measured via a hidden mirror so
// soft-wrapped lines count — a logical-line count undershoots the scroll badly for wrapped templates.
const caretOffsetTop = (textarea: HTMLTextAreaElement, position: number): number => {
const cs = getComputedStyle(textarea);
const mirror = document.createElement('div');
mirror.style.fontFamily = cs.fontFamily;
mirror.style.fontSize = cs.fontSize;
mirror.style.fontWeight = cs.fontWeight;
mirror.style.fontStyle = cs.fontStyle;
mirror.style.lineHeight = cs.lineHeight;
mirror.style.letterSpacing = cs.letterSpacing;
mirror.style.wordSpacing = cs.wordSpacing;
mirror.style.paddingTop = cs.paddingTop;
mirror.style.paddingRight = cs.paddingRight;
mirror.style.paddingBottom = cs.paddingBottom;
mirror.style.paddingLeft = cs.paddingLeft;
mirror.style.width = `${textarea.clientWidth}px`;
mirror.style.boxSizing = 'border-box';
mirror.style.whiteSpace = 'pre-wrap';
mirror.style.overflowWrap = 'break-word';
mirror.style.position = 'absolute';
mirror.style.visibility = 'hidden';
mirror.style.top = '-9999px';
mirror.style.left = '-9999px';
mirror.textContent = textarea.value.slice(0, position);
const marker = document.createElement('span');
marker.textContent = textarea.value.charAt(position) || '.';
mirror.appendChild(marker);
document.body.appendChild(mirror);
const offset = marker.offsetTop;
mirror.remove();
return offset;
};
interface DiffContentProps {
control: Control<HumanFormData> | Control<SystemFormData>;
oldValue: string;
styles: ComponentProps<typeof ReactDiffViewer>['styles'];
}
interface VariablesPanelContainerProps {
control: Control<HumanFormData> | Control<SystemFormData>;
onVariableClick: (variable: string) => void;
variables: string[];
}
interface VariablesProps {
currentTemplate: string;
onVariableClick: (variable: string) => void;
variables: string[];
}
// Don't hoist this useWatch to the parent — it would re-subscribe the whole page per keystroke.
function DiffContent({ control, oldValue, styles }: DiffContentProps) {
const newValue = useWatch({ control, name: 'template' });
return (
<ReactDiffViewer
newValue={newValue}
oldValue={oldValue}
splitView
styles={styles}
useDarkTheme
/>
);
}
function SettingsPrompt() {
const { promptId } = useParams<{ promptId: string }>();
const { isDesktop } = useBreakpoint();
@@ -335,60 +296,14 @@ function SettingsPrompt() {
const isLoading = isCreateLoading || isUpdateLoading || isDeleteLoading || isValidateLoading;
const handleVariableClick = useCallback(
(variable: string, field: { onChange: (value: string) => void; value: string }, formId: string) => {
if (viewMode === 'rich') {
if (!editorRef.current?.cycleToVariable?.(variable)) {
editorRef.current?.insertAtCursor?.(`{{.${variable}}}`);
}
return;
}
const textarea = document.querySelector(`#${formId} textarea`) as HTMLTextAreaElement;
if (textarea) {
const currentValue = field.value || '';
const variablePattern = `{{.${variable}}}`;
const matches = findVariableUseRanges(currentValue, variable);
if (matches.length > 0) {
const { selectionEnd, selectionStart } = textarea;
const currentIndex = matches.findIndex(
(match) => match.index === selectionStart && match.index + match.length === selectionEnd,
);
const target =
currentIndex >= 0
? matches[(currentIndex + 1) % matches.length]
: (matches.find((match) => match.index >= selectionStart) ?? matches[0]);
if (target) {
const matchStart = target.index;
textarea.focus();
textarea.setSelectionRange(matchStart, matchStart + target.length);
textarea.scrollTop = Math.max(
0,
caretOffsetTop(textarea, matchStart) - textarea.clientHeight / 2,
);
}
} else {
const start = textarea.selectionStart;
const end = textarea.selectionEnd;
const newValue =
currentValue.slice(0, Math.max(0, start)) +
variablePattern +
currentValue.slice(Math.max(0, end));
field.onChange(newValue);
setTimeout(() => {
textarea.focus({ preventScroll: true });
textarea.setSelectionRange(start + variablePattern.length, start + variablePattern.length);
}, 0);
}
}
},
[viewMode],
);
// The field's handle cycles/inserts in both raw and rich modes, so clicking a variable is mode-agnostic:
// jump to its next use, or insert `{{.Name}}` at the caret if it isn't used yet. `editorRef` points at the
// active tab's field (Radix unmounts the inactive tab), so this drives whichever prompt is on screen.
const handleVariableClick = useCallback((variable: string) => {
if (!editorRef.current?.cycleToVariable(variable)) {
editorRef.current?.insertAtCursor(`{{.${variable}}}`);
}
}, []);
const handleReset = () => {
setResetDialogOpen(true);
@@ -553,43 +468,19 @@ function SettingsPrompt() {
}
let variables: string[] = [];
let formId = '';
if (activeTab === 'system') {
variables =
promptInfo.type === 'agent'
? (promptInfo.data as AgentPrompt | AgentPrompts)?.system?.variables || []
: (promptInfo.data as DefaultPrompt)?.variables || [];
formId = 'system-prompt-form';
} else if (activeTab === 'human' && promptInfo.type === 'agent' && promptInfo.hasHuman) {
variables = (promptInfo.data as AgentPrompts)?.human?.variables || [];
formId = 'human-prompt-form';
}
return { formId, variables };
return { variables };
}, [promptInfo, activeTab]);
const handleVariableClickCallback = useCallback(
(variable: string) => {
if (!variablesData) {
return;
}
const field =
activeTab === 'system'
? {
onChange: (value: string) => systemForm.setValue('template', value, { shouldDirty: true }),
value: systemForm.getValues('template'),
}
: {
onChange: (value: string) => humanForm.setValue('template', value, { shouldDirty: true }),
value: humanForm.getValues('template'),
};
handleVariableClick(variable, field, variablesData.formId);
},
[activeTab, variablesData, systemForm, humanForm, handleVariableClick],
);
// Re-sync both tabs to the server prompt. A Save refetches settingsPrompts → promptInfo gets a new
// identity → this fires; keepDirtyValues (on both form configs) preserves the OTHER tab's unsaved edits,
// which an unguarded reset would silently wipe.
@@ -940,7 +831,7 @@ function SettingsPrompt() {
const variablesPanel = variablesData ? (
<VariablesPanelContainer
control={activeControl}
onVariableClick={handleVariableClickCallback}
onVariableClick={handleVariableClick}
variables={variablesData.variables}
/>
) : null;