Skip to content

fix(mothership): minor followups#3709

Merged
icecrasher321 merged 2 commits intostagingfrom
fix/mothership-followups
Mar 22, 2026
Merged

fix(mothership): minor followups#3709
icecrasher321 merged 2 commits intostagingfrom
fix/mothership-followups

Conversation

@icecrasher321
Copy link
Collaborator

Summary

  • Only manual aborts should abort
  • Diff engine acceptance of server side positions
  • Thinking indicator flash for copilot

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@cursor
Copy link

cursor bot commented Mar 22, 2026

PR Summary

Medium Risk
Changes stream lifecycle/abort behavior and message reconciliation during active SSE streaming, which can affect chat responsiveness and cancellation semantics. Also tweaks workflow autolayout heuristics, which could alter block positioning for new edits.

Overview
Reduces unintended copilot stream cancellations by removing automatic AbortController.abort() calls on home-page resets, component unmount, and server-side SSE stream cancel(); aborts are now primarily driven by explicit user stop/abort paths.

Improves chat UI stability during active streaming by preserving the local in-progress assistant message when refreshed chatHistory arrives for the same activeStreamId, avoiding a brief “thinking”/assistant message flash or replacement.

Adjusts workflow diff autolayout to respect server-provided positions for new blocks: newly introduced blocks are only included in client-side layout when their position is missing/invalid or still at the origin (0,0), reducing unexpected re-layout shifts.

Written by Cursor Bugbot for commit a3537e9. Configure here.

@vercel
Copy link

vercel bot commented Mar 22, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 22, 2026 9:22am

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 22, 2026

Greptile Summary

This PR ships three focused bug fixes for the copilot/mothership feature: (1) abort-scope narrowing so only explicit user-triggered stops cancel the server-side stream (enabling reconnect), (2) a flash-prevention guard that preserves the in-progress streaming assistant message when a server-pushed chatHistory update fires mid-stream, and (3) a diff-engine guard that skips client-side autolayout for new blocks that already carry a valid server-computed position.

  • use-chat.tsabortControllerRef.current?.abort() removed from the isHomePage guard and the cleanup effect; only stopGeneration (the manual path) still signals the AbortController. The new shouldPreserveActiveStreamingMessage flag prevents setMessages(mappedMessages) from replacing the live streaming bubble when the query cache refreshes mid-stream.
  • chat-streaming.tsabortController.abort() removed from the cancel() handler of the ReadableStream; client disconnect now only sets clientDisconnected = true, allowing the Sim→Go stream to finish and buffer its events for reconnect.
  • diff-engine.ts – New blocks whose position is already finite and non-origin (!(x===0 && y===0)) are excluded from the targeted autolayout pass, preventing spurious position shifts when block metrics change between edits.

Confidence Score: 4/5

  • Safe to merge; changes are narrow bug fixes with clear intent and no regressions on the primary user path.
  • All three changes are targeted and well-reasoned. The abort-scope narrowing and streaming-message preservation are logically sound for the reconnect pattern. The only non-blocking concerns are: (a) a stream that completes after client disconnect is recorded as 'cancelled' rather than 'completed', which could matter if reconnect logic gates on status; and (b) the (0, 0) origin guard in the diff engine is an implicit convention rather than an explicit contract. Neither is a regression or a blocker.
  • apps/sim/lib/copilot/chat-streaming.ts — verify that the reconnect path handles a 'cancelled'-status stream with a full snapshot identically to a 'completed' stream.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Two targeted fixes: removes non-manual abort calls (unmount and home-page-leave effects now null the ref without aborting, enabling server-side reconnect), and adds shouldPreserveActiveStreamingMessage logic to avoid flashing the thinking indicator when a server-pushed chatHistory update fires during an active stream.
apps/sim/lib/copilot/chat-streaming.ts Removes abortController.abort() from the ReadableStream cancel() handler so that client disconnect no longer prematurely kills the server-side Sim→Go stream, allowing the stream to complete and be buffered for reconnect.
apps/sim/lib/workflows/diff/diff-engine.ts New blocks with already-valid server-computed positions (non-finite or non-origin) are now excluded from the client-side autolayout pass, preventing redundant repositioning that could shift blocks when block metrics change between edits.

Sequence Diagram

sequenceDiagram
    participant U as User (Browser)
    participant UC as useChat Hook
    participant SSE as createSSEStream (Server)
    participant Go as Sim→Go Stream

    Note over U,Go: Before this PR — any disconnect aborted the server stream
    U->>UC: Navigate away / unmount
    UC->>SSE: streamReader.cancel()
    SSE->>Go: abortController.abort() ❌ (removed)
    Go-->>SSE: Processing stopped early

    Note over U,Go: After this PR — client disconnect lets stream finish for reconnect
    U->>UC: Navigate away / unmount
    UC->>SSE: streamReader.cancel()
    SSE->>SSE: clientDisconnected = true
    Go-->>SSE: Stream completes, events buffered
    SSE-->>SSE: setStreamMeta(status: cancelled*)

    U->>UC: Return to chat (reconnect)
    UC->>SSE: Reconnect with same streamId
    SSE-->>UC: Replay buffered snapshot events

    Note over UC: shouldPreserveActiveStreamingMessage
    UC->>UC: Check activeStreamId === streamIdRef
    UC->>UC: Preserve local streaming bubble,\navoid flash on chatHistory update

    Note over Go: *status 'cancelled' even on full completion when clientDisconnected
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/copilot/chat-streaming.ts, line 354-359 (link)

    P2 Server stream continues on client disconnect; stream marked cancelled on completion

    After this change, when the browser-side ReadableStream is cancelled (component unmount, navigation away, etc.), clientDisconnected = true is set but the underlying abortController is not signalled, so the Sim→Go processing continues to completion.

    However, once the stream finishes, lines 278-281 / 315-318 check if (clientDisconnected) and call setStreamMeta(streamId, { status: 'cancelled', ... }). This means a stream that ran to full completion while the client was away will be recorded as 'cancelled', not 'completed'. If any reconnect or polling logic downstream gates on a 'completed' status, reconnecting clients may not find the finished result. Worth verifying that the reconnect path handles 'cancelled' + a fully-populated snapshot the same way it handles 'completed'.

Reviews (1): Last reviewed commit: "diff engine fix" | Re-trigger Greptile

@icecrasher321 icecrasher321 merged commit 4cb5e34 into staging Mar 22, 2026
12 checks passed
@icecrasher321 icecrasher321 deleted the fix/mothership-followups branch March 22, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant