AI-First Starting UX: Build with AI path#4902
Conversation
…I panel, suppress legacy placeholder
…lose button, disable ⌘K
…ew workflows When YAML validation fails (syntax, ID format, schema) during handleApplyWorkflow and the workflow is new, inject the error as an assistant message into the AI chat thread instead of showing a toast. Save failures keep the existing toast behaviour.
Add tests for the six untested behaviours introduced in #4868: - auto-save called after importWorkflow when isNewWorkflow - validation errors routed to onValidationError callback, not toast - save failures show toast and do not invoke onValidationError - close button absent when onClose is undefined - empty-canvas placeholder suppressed when AI panel is open Also adds saveWorkflow to mockWorkflowActions and isNewWorkflow: false to all existing handleApplyWorkflow renderHook calls to match the updated hook interface.
…e fails When saveWorkflow rejects on a new workflow, fit-view was incorrectly dispatched after doneApplyingWorkflow, implying the workflow was successfully persisted when it wasn't. Add a saveSucceeded flag and skip fit-view when save fails. Also wires up the Retry action on the save-failure toast so the user can re-attempt the full apply+save without losing their canvas state.
…ies workflow The workflow was being applied twice — once via streaming and again when the final new_message arrived — causing a transient dirty state (unsaved red dot) between the second import and save. Consolidate the deduplication flag into useAIWorkflowApplications so the "mark as applied + skip" logic happens atomically in one place rather than split across two separate useEffect passes in the wrapper. Test changes: delete a duplicate test identical to an existing case, tighten the streaming-skip assertion to also verify saveWorkflow is not called.
…fix stale Retry closure
On /new, URL params like ?chat=true, ?method=template, ?panel=run, ?panel=editor, and ?panel=settings could bypass the landing screen or open panels that shouldn't be reachable before the user takes an action. - createUIStore: return clean defaults when isNewWorkflow=true so the store ignores all URL params at init time - WorkflowEditor: guard the URL->panel sync effect with isNewWorkflow so ?panel=run can't open the run panel - WorkflowEditor: short-circuit isIDEOpen and showInspector on new workflows so FullScreenIDE (z-50) and Inspector can't render above the landing screen via URL params - Add TODO-AI-FIRST annotations on the method URL sync and old placeholder block for cleanup when the left-panel flow is removed
…g YAML When the initial save fails after AI applies a workflow, the Retry button was re-running the full handleApplyWorkflow (importWorkflow + save). Add a saveWorkflowRef that points at the latest saveWorkflow callback so Retry only calls save, skipping the already-successful canvas apply. Also set duration: Infinity on the save-failure toast so it persists until the user explicitly retries or navigates away.
…ntent The error rendering for assistant messages changed: non-empty error content now renders inline in a styled red box (ai-validation-error) rather than showing the hardcoded 'Failed to send message' banner. Split the single test into two cases — one for non-empty content (styled box) and one for empty content (banner).
…ations test fixtures Both test files were missing the now-required saveWorkflow field in mockWorkflowActions and the required isNewWorkflow boolean at every renderHook call site, causing TypeScript compile errors.
… failure When streaming fires mid-conversation and handleApplyWorkflow fails (e.g. invalid YAML, importWorkflow throws), appliedViaStreamingRef was left as true. The session-load guard only runs once per session, so the next real new_message would hit the early-return and the workflow would silently never be applied. Reset the ref in the catch block so a failed streaming apply doesn't block the subsequent settled-message auto-apply.
…toast The save-failure Retry onClick used void to discard the promise, so a second save failure was silently swallowed with no user feedback. Chain a .catch on the retry promise to show another alert if the retry also fails.
… channel join createUIStore initialises once inside useState — if the channel hasn't joined by mount time, isNewWorkflow is false and showLandingScreen was permanently frozen as false. Initialize showLandingScreen to true and move the isNewWorkflow gate into useShowLandingScreen, where it reads from the reactive SessionContextStore. On slow connections the landing screen now correctly appears once the channel joins and delivers isNewWorkflow=true.
…on disconnect saveWorkflow returns null (not throws) when the WebSocket is disconnected. The null return was silently treated as success — saveSucceeded stayed true, fit-view fired, and the landing screen stayed dismissed. Check the return value and throw on null so the existing catch path handles it: shows the save-failure toast with a Retry button and sets saveSucceeded to false. The retry path is also fixed to handle null via an async IIFE.
…failure after streaming apply When importWorkflow succeeded but saveWorkflow subsequently failed, the inner catch set saveSucceeded=false but never reset appliedViaStreamingRef. The outer catch (which does reset it) was never reached, leaving the ref true. The next confirmed message from the server was then silently skipped by the auto-apply effect. Reset the ref in the inner save-failure catch, mirroring the existing reset in the outer catch. Covers both thrown errors and null returns (disconnect).
…wRef The ref was assigned but never called — the Retry toast uses saveWorkflowRef and callers receive handleApplyWorkflow directly from the hook return value.
|
All 15 changed files are frontend-only (TypeScript/React in Security Review ✅
|
|
Hey @lmac-1 popped together some looms for design review: No Ai : https://www.loom.com/share/6d76a5aea6e54a0bab8a577b6d432d26 |
There was a problem hiding this comment.
Really nice work Lucy 🙏 The Build with AI flow feels good to use and the tests are genuinely thorough. I left a fair few inline comments but please don't be scared by the count - most are questions or non-blocking. Three I'd love us to sort before this merges:
- Double toast on save failure - the wrapped
saveWorkflowalready shows its own error toast and re-throws, so we get two for one failure. crypto.randomUUID- I think this can throw on a self-hosted HTTP deploy. We already have arandomUUID()helper incommon.jsfor exactly this.- Getting stuck in the panel - if the auto-save keeps failing, there's no close button and no way back to the canvas. Not sure of the best fix, keen to hear your thoughts.
Everything else is either a question for you / product (auto-saving refinements, the panel escape) or fits nicely into the cleanup issue you already planned, so no rush there.
And your point 4 about the ref churn is spot on - I came at it from a few different angles and kept landing on the same thing. I think a small state machine would kill a whole class of these quietly-skipped bugs. Really good instinct flagging it.
Not blocking the epic branch in my view once those three are handled. Thanks again, I really enjoyed reviewing this one 🙌
| } catch (saveError) { | ||
| saveSucceeded = false; | ||
| if (appliedViaStreamingRef) appliedViaStreamingRef.current = false; | ||
| console.error('[AI Assistant] Failed to save workflow:', saveError); | ||
| notifications.alert({ | ||
| title: 'Failed to save workflow', | ||
| description: | ||
| saveError instanceof Error | ||
| ? saveError.message | ||
| : 'Unknown error occurred', | ||
| duration: Infinity, // toast only dismisses by clicking 'x' so the user definitely sees the error | ||
| action: { | ||
| label: 'Retry', | ||
| onClick: () => { | ||
| void (async () => { | ||
| try { | ||
| const saved = await saveWorkflowRef.current?.({ | ||
| silent: true, | ||
| }); | ||
| if (!saved) | ||
| throw new Error( | ||
| 'Your connection was lost. Reconnect and try again.' | ||
| ); | ||
| } catch (retryError: unknown) { | ||
| console.error( | ||
| '[AI Assistant] Retry save failed:', | ||
| retryError | ||
| ); | ||
| notifications.alert({ | ||
| title: 'Failed to save workflow', | ||
| description: | ||
| retryError instanceof Error | ||
| ? retryError.message | ||
| : 'Unknown error occurred', | ||
| }); | ||
| } | ||
| })(); | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ah, I think we get two toasts here for a single failure. The saveWorkflow we call is the wrapped one from useWorkflowActions, and that wrapper already pops its own "Failed to save" alert (with a Retry) and then re-throws - so this catch lands a second one on top. The { silent: true } only mutes the success toast sadly, not the error path. The disconnect case is actually fine, the wrapper returns null and we throw the connection-lost message ourselves. So maybe we keep just the !saved branch here and let the wrapper own the real errors ? Side note while I was in here: the new test stubs saveWorkflow as a bare reject, which skips the wrapper, so it never catches this.
| const onValidationError = useCallback( | ||
| (errorMessage: string) => { | ||
| const message: Message = { | ||
| id: crypto.randomUUID(), |
There was a problem hiding this comment.
This one caught my eye. I'm fairly sure crypto.randomUUID will throw on a self-hosted HTTP box - it's only defined in a secure context (HTTPS or localhost), so on a plain LAN deploy it's undefined, and here it throws before the message ever gets added. So when the AI returns invalid YAML the user just sees... nothing. We already dodge this everywhere else with the randomUUID() helper in common.js (Math.random based, see WorkflowDiagram.tsx / useConnect.ts). Could we reuse that here ?
| <AIAssistantPanel | ||
| isOpen={isAIAssistantPanelOpen} | ||
| onClose={handleClosePanel} | ||
| onClose={isNewWorkflow ? undefined : handleClosePanel} |
There was a problem hiding this comment.
Something I'd like to think through with you: I think a user can get trapped here if the auto-save fails. isNewWorkflow only flips to false on a successful save (clearIsNewWorkflow, useWorkflow.tsx:436), so while it keeps failing the panel has no close button (onClose is undefined), Cmd+K is off, and the canvas is gated behind the new-workflow guards. Their only escape is the Retry toast or a full reload. Is that what we want ? Feels like they should at least be able to close the panel or drop back to the landing screen when the save hasn't gone through. I honestly don't have a clean fix in mind - curious what you think.
| if (isNewWorkflow) { | ||
| try { | ||
| const saved = await saveWorkflow({ silent: true }); | ||
| if (!saved) { | ||
| throw new Error( | ||
| 'Your connection was lost. Reconnect and try again.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
More of a question than a problem. Once the first save happens, clearIsNewWorkflow() runs and isNewWorkflow is false from then on. So the very first AI message auto-saves, but every refinement after it ("add a second job") imports onto the canvas and just leaves the unsaved dot - nothing persists until they hit Save. It matches the normal editor so it's defensible, but inside this AI-first flow it feels a little inconsistent, and I'm not sure an AI-first user would realise they need to save. Intended for now, or should refinements save too ? Probably a product call.
| // Streaming already applied this YAML — skip the re-import to avoid | ||
| // a transient dirty state (Y.Doc write → unsaved red dot) between | ||
| // the import and save that would otherwise follow. | ||
| if (appliedViaStreamingRef?.current) { | ||
| appliedViaStreamingRef.current = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Non-blocking, and honestly more of a nagging worry than a bug I could reproduce. This skip depends on the ref being cleared on every path. If streaming applies the YAML (ref = true) but the matching new_message never comes back as success + code, the ref stays true and the next unrelated response gets skipped right here, silently - no import, no error. It's basically the churn you called out in point 4, four reset sites is a lot to keep straight. A little state machine (or having handleApplyWorkflow return success and setting the ref from that) would make the worry go away. Totally happy to leave it for the follow-up though.
| // If streaming set this ref before the apply failed, clear it so the | ||
| // next real new_message isn't silently skipped by the auto-apply guard. | ||
| if (appliedViaStreamingRef) appliedViaStreamingRef.current = false; | ||
|
|
||
| const errorMessage = | ||
| error instanceof Error ? error.message : 'Invalid workflow YAML'; | ||
|
|
||
| if (isNewWorkflow && onValidationError) { | ||
| onValidationError(errorMessage); | ||
| } else { | ||
| notifications.alert({ | ||
| title: 'Failed to apply workflow', | ||
| description: errorMessage, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think invalid YAML ends up posting two identical error bubbles. Streaming tries it first, fails, and this catch resets the ref and fires onValidationError (bubble 1). Then the final new_message shows up with the same YAML, but the ref is false again so the skip guard at line 515 lets it through - it re-applies, fails, bubble 2. In the happy path that guard is exactly what stops the double apply, we just lose it on the failure path. Did you bump into this when you were testing the invalid-YAML case ?
| isNewWorkflow, | ||
| onValidationError, | ||
| saveWorkflow, | ||
| ] | ||
| ); | ||
|
|
||
| // Keep ref pointing at the latest callback so Retry toast closures never go stale | ||
| saveWorkflowRef.current = saveWorkflow; |
There was a problem hiding this comment.
Tiny perf nit, non-blocking. saveWorkflow from useWorkflowActions is a fresh function every render (it's an IIFE at useWorkflow.tsx:393 and the hook isn't memoized), so pulling it into these deps makes handleApplyWorkflow new each render, which makes the auto-apply effect tear down and re-run every render - even on every streaming token. No correctness issue since appliedMessageIdsRef dedupes, it's just churn. Since you already keep saveWorkflowRef around, maybe the callback reads saveWorkflowRef.current(...) and we drop saveWorkflow from the deps ?
| {message.status === 'error' && | ||
| !isStreaming(message) && | ||
| message.content.trim() ? ( | ||
| <div | ||
| className="rounded-lg border border-red-200 bg-red-50 px-3 py-2" | ||
| data-testid="ai-validation-error" | ||
| > | ||
| <div className="flex items-start gap-2"> | ||
| <span className="hero-exclamation-circle h-4 w-4 text-red-600 flex-shrink-0 mt-0.5" /> | ||
| <p className="text-sm text-red-700 leading-relaxed"> | ||
| {message.content} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
I think this quietly drops the Retry button for genuine send failures that happen to have text. Before, any assistant message with status: 'error' got the "Failed to send / Retry" banner. Now anything with content lands in this read-only red box, and the banner underneath is gated on !content.trim(). So a generation that dies after streaming a bit of text (it gets marked error over in AIChannelRegistry.ts:682) shows up here with no way to retry. I wonder if an explicit field like errorKind: 'validation' | 'transport' would be safer than leaning on content.trim() to tell them apart ? Not a blocker for this PR though.
| @@ -1 +1 @@ | |||
| /** | |||
There was a problem hiding this comment.
Last one, promise 😄 I only noticed because I was scrolling for a while - this file has crept up to 700 lines. That's quite big. Nothing for now, but maybe in the cleanup pass we peel the save / streaming cases out into their own file ?
… store state
appliedViaStreamingRef needed manual resets at four separate failure
sites and was invisible outside the component. Replace it with a
streamingApply record ({ yaml, saveFailed }) on AIAssistantStore,
written only after a successful streaming import, so failed applies
never leave a stale flag behind.
The auto-apply effect now compares the final message's YAML against
the record instead of checking a boolean: matching content and a
settled save means skip; matching content with a save still owed
means retry the save without re-importing; anything else applies
normally.
Also give the "failed to save" toast a stable id so repeated
failures update it in place instead of stacking duplicates.
3 participants
What I need in this review: Focus on the "Build with AI" path of the epic. Any big code organisation problems will be fixed in the clean up issue.
Description
This PR adds the "Build with AI path" of the #4848 AI-First Starting UX epic.
How it works:
sent
Key decisions:
appliedViaStreamingRef. The streamingstreamingChangesevent and the finalnew_messageboth carry the same YAML. The ref is set before the streaming apply and cleared by the auto-apply effect after skipping, preventing a double import. The ref is also reset on apply or save failure so thenew_messagepath can retry if streaming failed.?panel=run,?panel=editor,?panel=settings, and?chat=truecould open panels or bypass the landing screen before the user has done anything. The store ignores all URL params at init time whenisNewWorkflow=true, andWorkflowEditorguards its URL→panel sync effect with the same flag. This prevents e.g. someone sharing a/new?panel=runlink that skips straight to the run panel on a blank workflow.Closes #4868
Validation steps
/new, type a prompt, press Enter — confirm landing screen dismisses, AI panel opens with prompt pre-sent/w/{id}without a reloadAdditional notes for the reviewer
createUIStoreis created once insideuseState— on a slow connection the channel hasn't joined yet, so isNewWorkflow is false at init time. InitialisingshowLandingScreen: truealways and moving the isNewWorkflow guard intouseShowLandingScreen(which reads from the reactiveSessionContextStore) means the landing screen correctly appears once the channel joins, even on slow connections.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)