Skip to content

Conversation

@mmabrouk
Copy link
Member

@mmabrouk mmabrouk commented Feb 10, 2026

Summary

Stacked on #3687 (backend).

  • Adds a two-panel Refine Prompt modal to the Playground, triggered by a magic wand icon on each prompt header
  • Left panel: Chat-like instructions interface using @ant-design/x Sender + Prompts + Bubble components, with a predefined "Optimize using best practices" quick-action
  • Right panel: Editable refined messages preview with JSON diff toggle (DiffView)
  • Backend schema change: Simplified output to { messages, summary } — removed redundant refined_prompt field; summary is displayed as the AI chat reply
  • Bug fix: Preserves original template_format through refinement (only message content is changed, diff now compares only messages)
  • Keyboard shortcut Cmd/Ctrl+Enter to submit from anywhere in the modal
  • Accessibility: aria-label on icon buttons, switch, close button; aria-hidden on decorative icons; overscroll-behavior: contain on modal
  • State managed via Jotai atomFamily atoms (per-prompt scoped); destroyOnClose resets all state
  • "Use Refined Prompt" writes back to playground via moleculeBackedPromptsAtomFamily

Test plan

  • Open Playground, verify magic wand icon appears on prompt header when AI services are enabled
  • Click wand → modal opens at ~90vh height with two panels
  • Type instructions or click "Optimize the prompt using best practices" → left panel shows chat bubbles, right panel shows refined messages
  • Toggle Diff switch → shows JSON diff of messages only (no template_format noise)
  • Edit refined messages in the right panel
  • Click "Use Refined Prompt" → modal closes, playground prompt updates with refined content
  • Verify template_format is preserved (not changed to fstring)
  • Test Cmd+Enter triggers refinement from the input
  • Test iterative refinement (refine → adjust instructions → refine again)
CleanShot.2026-02-10.at.19.39.34.mp4

Open with Devin

…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
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Feb 10, 2026
@vercel
Copy link

vercel bot commented Feb 10, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview, Comment Feb 10, 2026 3:10pm

Request Review

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 9 additional findings.

Open in Devin Review

Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +56 to +69
useEffect(() => {
let mounted = true
aiServicesApi
.getStatus()
.then((status) => {
if (mounted) setAiServicesEnabled(status.enabled)
})
.catch(() => {
if (mounted) setAiServicesEnabled(false)
})
return () => {
mounted = false
}
}, [])
Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +62 to +72
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})
}, [])
Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +77 to +97
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,
},
}
})
})
Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +138 to +144
<Switch
size="small"
checked={showDiff}
onChange={setShowDiff}
disabled={!hasRefinements}
aria-label="Toggle diff view"
/>
Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +162 to +165
// Capture original snapshot on first refinement
if (!originalSnapshotRef.current) {
setOriginalSnapshot(promptToRefine)
}
Copy link
Contributor

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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@mmabrouk mmabrouk requested a review from ardaerzin February 10, 2026 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant