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.
| <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.
There was a problem hiding this comment.
Hey @elias-ba , thanks for raising this. I double checked with product and they are happy with a generic response "Failed to save. Please retry or try again later. [Retry]" with no way to get out. Brandon said :
"report this as a critical error in our telemetry system since if the AI assistant returned a response and this failed, that suggests there isn’t a general network problem - there is a problem on our end which requires our urgent attention"
but I'm not sure we can trigger Sentry errors from the frontend? What do you think? I think for now it's something that we can work on to improve but shouldn't block things.
There was a problem hiding this comment.
On the Sentry question: we can report from the frontend. We already run Sentry.init() in app.js through @sentry/react, so a Sentry.captureException / captureMessage call is available client side. That means Brandon's idea of flagging this as a critical error is doable whenever we pick up the follow-up. Agreed it shouldn't block this one.
| // 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.
There was a problem hiding this comment.
I have committed a fix to make this a lot cleaner and stem from the store. So I think this is okay now.
| // 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 ?
There was a problem hiding this comment.
Yes. I have it noted as a "nice-to-have" to fix. I spent some time today trying to improve this but it seems to be quite a gnarly problem and not straightforward. I think I will have to note it for a future improvement. Hopefully it won't be visible very often, and the user will 'just' see an extra toast message. Noisy but not the end of the world. I would rather that than rushing a fix that could have implications for other parts of the app using these silent saves.
I have added a success toast though if the Retry passes, which I think is a better experience.
| {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 ?
There was a problem hiding this comment.
Yes will note it to clean up in the cleanup PR, thanks!
… 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.
….randomUUID crypto.randomUUID is only defined in secure contexts (HTTPS or localhost), so it throws on self-hosted HTTP deploys. Swap to the existing Math.random-based randomUUID() helper from common.js, already used elsewhere (WorkflowDiagram.tsx, useConnect.ts).
The wrapped saveWorkflow (useWorkflow.tsx) already shows its own "Failed to save workflow" toast and re-throws for real save errors. saveNewWorkflow's catch block was showing a second one on top of it. Split the disconnect case (saveWorkflow returns null, wrapper shows nothing) from the thrown-error case (wrapper already handles it) so only one toast ever fires per failure.
|
Hey @elias-ba thank you so much for your timely and thorough review. This was so helpful. Here's where I got to with your 3 items:
Also I cleaned up the I think we are good to merge for now and work out the other bits later on. I'll come back to your comments when I'm doing a more general sweep. Thanks again. |
elias-ba
left a comment
elias-ba
left a comment
There was a problem hiding this comment.
Thanks Lucy, all three are sorted on my end. Approving 🙌
useAIWorkflowApplications requires a streamingApplyActions object since #4902 added it, but this test file's renderApplications helper never passed one, so handleApplyWorkflow crashed on streamingApplyActions.clear() being undefined. The three sibling test files (autoApply, jobCode, workflow) were updated in #4902; this one was missed.
* Remove AI disclaimer gate from collaborative editor Delete the blocking DisclaimerScreen and all has_read_ai_disclaimer / hasReadDisclaimer state, props, hooks, and channel-registry plumbing. The AI assistant panel now opens straight into chat. Backend still sends the disclaimer fields on the wire at this point; that follow-up removal (plus the legacy LiveView gate) lands in a separate commit. * Remove backend AI disclaimer tracking and legacy onboarding gate Delete the 24-hour disclaimer acknowledgement tracking (user_has_read_disclaimer?/1, mark_disclaimer_read/1), the channel fields/handlers that exposed it, and the legacy LiveView onboarding screen. The frontend gate was already removed in a prior commit; this strips the now-unused backend support and adds a migration to clear the stale ai_assistant.disclaimer_read_at preference from existing users. * Remove unused ChatInterface component * Add AI disclaimer footer to chat input and landing screen * Add E2E coverage for AI disclaimer footer on landing screen Add a test-id to AIDisclaimerFooter so it can be located in Playwright, expose locators on WorkflowEditPage, and assert the footer's presence (with correct link href/target) when AI is enabled, and its absence when AI is disabled. * Hide Conversations menu item during new workflow creation A new, unsaved workflow has no session history to show, so offering "Conversations" in the AI panel's three-dots menu just leads to an empty list. Gate it on isNewWorkflow, matching the pattern already used elsewhere (Header, CollaborativeEditor) to hide UI until the first save. Also wire a mocked sessionContextStore into the panel's test harness, since the component now reads isNewWorkflow from it. * updates changelog * Clean up remaining traces of the AI disclaimer removal - Fix AIDisclaimerFooter icon: hero-exclamation-triangle doesn't exist as a heroicon, use hero-shield-exclamation instead - Remove useSessionContextLoaded, an orphaned hook whose only caller was the disclaimer-gating logic removed in #4911 - Update store-structure.md to drop references to the deleted hasReadAIDisclaimer/markAIDisclaimerRead/hasReadDisclaimer/ markDisclaimerRead store fields and commands * Fix missing streamingApplyActions mock in globalStep tests useAIWorkflowApplications requires a streamingApplyActions object since #4902 added it, but this test file's renderApplications helper never passed one, so handleApplyWorkflow crashed on streamingApplyActions.clear() being undefined. The three sibling test files (autoApply, jobCode, workflow) were updated in #4902; this one was missed.
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)