-
Notifications
You must be signed in to change notification settings - Fork 478
feat(frontend): Refine Prompt modal — AI-powered prompt refinement UI #3698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/refine-ai-feature
Are you sure you want to change the base?
Conversation
…ment Two-panel modal (Instructions + Preview) integrated into the Playground: - Left panel: @ant-design/x Sender + Prompts for guidelines input with chat-like display - Right panel: Editable refined messages with JSON diff toggle - Backend output schema simplified: messages + summary (removed redundant refined_prompt) - Preserves original template_format through refinement (only messages are changed) - Keyboard shortcut (Cmd/Ctrl+Enter), accessibility (aria-labels, focus management) - Magic wand icon on prompt header with AI services availability check
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Backend schema change: removed refined_prompt field, frontend spec doc is stale
The backend service.py and spec.md were updated to remove the refined_prompt field and add summary. The frontend implementation in parseRefineResponse (useRefinePrompt.ts:81-102) and the API client (api.ts:34-36) correctly use the new messages + summary schema. However, docs/design/ai-actions/frontend-spec.md still references the old schema in several places (lines 29, 462-466, 542, 548, 674), mentioning refined_prompt and explanation fields that no longer exist in the actual implementation. While this is documentation-only, stale specs can mislead future contributors.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| let mounted = true | ||
| aiServicesApi | ||
| .getStatus() | ||
| .then((status) => { | ||
| if (mounted) setAiServicesEnabled(status.enabled) | ||
| }) | ||
| .catch(() => { | ||
| if (mounted) setAiServicesEnabled(false) | ||
| }) | ||
| return () => { | ||
| mounted = false | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Per-instance getStatus() API call violates data fetching conventions
Each PlaygroundVariantConfigPromptCollapseHeader instance fires its own aiServicesApi.getStatus() call on mount via useEffect. In a playground with N variants × M prompts, this results in N×M identical API calls to the same endpoint. The AGENTS.md explicitly says to avoid useEffect for data fetching and recommends atomWithQuery from jotai-tanstack-query with proper caching/deduplication. A single atomWithQuery for the status endpoint (or even an atomFamily-backed query) would deduplicate these calls and provide proper caching via staleTime. Additionally, during loading (aiServicesEnabled === null), the button shows "AI services not available" tooltip which is misleading — it should show a loading state or no tooltip.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| const handler = (e: KeyboardEvent) => { | ||
| if ((e.metaKey || e.ctrlKey) && e.key === "Enter") { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| submitRef.current?.() | ||
| } | ||
| } | ||
| document.addEventListener("keydown", handler, {capture: true}) | ||
| return () => document.removeEventListener("keydown", handler, {capture: true}) | ||
| }, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Cmd+Enter handler unconditionally swallows events at capture phase
This handler registers on document with {capture: true} and calls both e.preventDefault() and e.stopPropagation() before invoking submitRef.current?.(). The prevent/stop calls happen unconditionally — even when submitRef.current is null (race on mount) or when handleCmdEnterSubmit returns early (empty input / loading). This means ALL Cmd+Enter keydowns are swallowed while the modal is mounted, preventing any other component (including the Sender component's own Enter handling, or any future keyboard shortcut) from receiving the event. Since destroyOnClose ensures this is only active while the modal is open and the modal is a blocking overlay, this is acceptable in practice but fragile. A safer approach would be to call preventDefault/stopPropagation only after confirming the submit will actually proceed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| setPrompts((prevPrompts: any[]) => { | ||
| return prevPrompts.map((prompt: any) => { | ||
| if (prompt?.__id !== promptId && prompt?.__name !== promptId) { | ||
| return prompt | ||
| } | ||
|
|
||
| const wrappedMessages = workingPrompt.messages.map((msg) => ({ | ||
| __id: generateId(), | ||
| role: {value: msg.role}, | ||
| content: {value: msg.content}, | ||
| })) | ||
|
|
||
| return { | ||
| ...prompt, | ||
| messages: { | ||
| ...prompt.messages, | ||
| value: wrappedMessages, | ||
| }, | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Immer recipe returns new value via .map() — works but breaks convention
The setPrompts call at line 77 passes a function that uses .map() and returns a new array rather than mutating the Immer draft in-place. The moleculeBackedPromptsAtomFamily write path (legacyEntityBridge.ts:317) forwards function updates to mutateEnhancedPromptsAtom which calls produce(currentPrompts, recipe). Immer's produce does support returning a new value from a recipe (it uses the returned value as the replacement), so this works at runtime. However, the type signature is recipe: (draft: unknown[]) => void (see store.ts:997), and existing usages in the codebase (e.g., promptTemplateFormat.ts:96) use Immer's mutation style (draft.forEach(...)) rather than returning. This divergence from the established convention could confuse future maintainers who expect mutation-only recipes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <Switch | ||
| size="small" | ||
| checked={showDiff} | ||
| onChange={setShowDiff} | ||
| disabled={!hasRefinements} | ||
| aria-label="Toggle diff view" | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Diff toggle enabled after error-only iterations shows inconsistent UI
After a refinement error on the first attempt, iterations.length > 0 so hasRefinements = true, but workingPrompt stays null (it's only set on success). The diff toggle at line 142 uses disabled={!hasRefinements} so it becomes enabled. However, in PreviewPanel.tsx:69, the null guard if (!workingPrompt) returns a static message before the diff/edit logic runs — so the enabled toggle has no visible effect. The toggle appears interactive but does nothing, which is confusing. Using disabled={!hasRefinedPrompt} (which checks workingPrompt !== null && iterations.length > 0) would be more accurate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Capture original snapshot on first refinement | ||
| if (!originalSnapshotRef.current) { | ||
| setOriginalSnapshot(promptToRefine) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Original snapshot set before API call — persists even on first-attempt error
At line 163, setOriginalSnapshot(promptToRefine) is called before the API call at line 167. If the API call fails, the original snapshot is set but workingPrompt remains null. This means the original snapshot captures the prompt state at the time of the first attempt, which is correct for diff comparison. On subsequent successful attempts, the snapshot is not overwritten because of the !originalSnapshotRef.current guard. However, the originalSnapshotRef is synced from the atom value on the next render — between the setOriginalSnapshot call and the next render, the ref is still null. The isLoading guard in handleSubmit prevents concurrent calls, so there's no race condition here. This is safe but worth noting for maintainers.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Stacked on #3687 (backend).
@ant-design/xSender + Prompts + Bubble components, with a predefined "Optimize using best practices" quick-action{ messages, summary }— removed redundantrefined_promptfield;summaryis displayed as the AI chat replytemplate_formatthrough refinement (only message content is changed, diff now compares only messages)Cmd/Ctrl+Enterto submit from anywhere in the modalaria-labelon icon buttons, switch, close button;aria-hiddenon decorative icons;overscroll-behavior: containon modalatomFamilyatoms (per-prompt scoped);destroyOnCloseresets all statemoleculeBackedPromptsAtomFamilyTest plan
template_formatnoise)template_formatis preserved (not changed tofstring)Cmd+Entertriggers refinement from the inputCleanShot.2026-02-10.at.19.39.34.mp4