From 2e9eb96f7562e6a7442661002edbb6327fbe1f4d Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Sun, 5 Jul 2026 13:21:35 +0700 Subject: [PATCH] refactor(markdown-editor): complete the field handle in raw mode, delete the settings-prompt bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../shared/markdown-editor/index.ts | 1 - .../markdown-editor-field.test.tsx | 106 ++++++++++ .../markdown-editor/markdown-editor-field.tsx | 47 +++-- .../markdown-editor/markdown-editor-styles.ts | 4 + .../markdown-editor-textarea.ts | 77 +++++++ .../markdown-editor-variable-syntax.ts | 20 ++ .../markdown-editor/markdown-editor.tsx | 35 ++-- .../src/pages/settings/settings-prompt.tsx | 195 ++++-------------- 8 files changed, 302 insertions(+), 183 deletions(-) create mode 100644 frontend/src/components/shared/markdown-editor/markdown-editor-field.test.tsx create mode 100644 frontend/src/components/shared/markdown-editor/markdown-editor-styles.ts create mode 100644 frontend/src/components/shared/markdown-editor/markdown-editor-textarea.ts diff --git a/frontend/src/components/shared/markdown-editor/index.ts b/frontend/src/components/shared/markdown-editor/index.ts index f0d84384..25605a84 100644 --- a/frontend/src/components/shared/markdown-editor/index.ts +++ b/frontend/src/components/shared/markdown-editor/index.ts @@ -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). diff --git a/frontend/src/components/shared/markdown-editor/markdown-editor-field.test.tsx b/frontend/src/components/shared/markdown-editor/markdown-editor-field.test.tsx new file mode 100644 index 00000000..47d733ce --- /dev/null +++ b/frontend/src/components/shared/markdown-editor/markdown-editor-field.test.tsx @@ -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(); + const { container } = render( + , + ); + 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(); + const { container } = render( + , + ); + 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(); + render( + , + ); + + const richRef = createRef(); + const { container } = render( + , + ); + 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'); + } + }); +}); diff --git a/frontend/src/components/shared/markdown-editor/markdown-editor-field.tsx b/frontend/src/components/shared/markdown-editor/markdown-editor-field.tsx index d1d212df..caf50f70 100644 --- a/frontend/src/components/shared/markdown-editor/markdown-editor-field.tsx +++ b/frontend/src/components/shared/markdown-editor/markdown-editor-field.tsx @@ -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 { - // Sizes the field's outer box in both modes — the rich editor wrapper and the raw