fix(native-chat): scope composer sends to the active turn - #8568
Conversation
📝 WalkthroughWalkthroughNative chat send functions now return cancellable handles with settlement timing. A lifecycle hook tracks and cancels pending sends across interruptions, PTY changes, unmounts, and settlement. Pending message matching now uses content keys, transcript boundaries, and occurrence counts for text and image submissions. Composer and view logic use this lifecycle, remove canceled optimistic entries, and include pending messages in streaming derivation. Tests cover cancellation, lifecycle behavior, occurrence reconciliation, pruning, visibility, and streaming previews. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/native-chat/NativeChatView.tsx (1)
309-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
pendingSendsAsMessagescomputation.
pendingSendsAsMessages(pending, sessionAfterCommandBoundaries.messages)is called twice with identical arguments — once inside thestreamingTextmemo (Line 310) and again insidesessionWithPending(Line 330). Consider computing it once (e.g. in its ownuseMemo) and reusing the result in both places to avoid recomputation and future drift between the two call sites.♻️ Proposed refactor
+ const pendingMessages = useMemo( + () => pendingSendsAsMessages(pending, sessionAfterCommandBoundaries.messages), + [pending, sessionAfterCommandBoundaries.messages] + ) const streamingText = useMemo(() => { - const pendingMessages = pendingSendsAsMessages(pending, sessionAfterCommandBoundaries.messages) return deriveNativeChatStreamingText({ messages: pendingMessages.length > 0 ? [...sessionAfterCommandBoundaries.messages, ...pendingMessages] : sessionAfterCommandBoundaries.messages, previewText: hookPreview, working: hookWorking }) - }, [sessionAfterCommandBoundaries.messages, pending, hookPreview, hookWorking]) + }, [sessionAfterCommandBoundaries.messages, pendingMessages, hookPreview, hookWorking]) const sessionWithPending = useMemo<typeof session>(() => { if (pending.length === 0 && commandMarkers.length === 0 && !streamingText) { return sessionAfterCommandBoundaries } return { ...sessionAfterCommandBoundaries, messages: [ ...sessionAfterCommandBoundaries.messages, ...commandMarkersAsMessages(commandMarkers), ...(streamingText ? [nativeChatStreamingMessage(streamingText)] : []), - ...pendingSendsAsMessages(pending, sessionAfterCommandBoundaries.messages) + ...pendingMessages ] } - }, [sessionAfterCommandBoundaries, pending, commandMarkers, streamingText]) + }, [sessionAfterCommandBoundaries, pendingMessages, commandMarkers, streamingText])
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4dd7623a-e123-427b-b36c-f66af9f1715b
📒 Files selected for processing (14)
src/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/NativeChatView.tsxsrc/renderer/src/components/native-chat/native-chat-pending-occurrence.test.tssrc/renderer/src/components/native-chat/native-chat-pending-occurrence.tssrc/renderer/src/components/native-chat/native-chat-pending.test.tssrc/renderer/src/components/native-chat/native-chat-pending.tssrc/renderer/src/components/native-chat/native-chat-runtime-send.test.tssrc/renderer/src/components/native-chat/native-chat-runtime-send.tssrc/renderer/src/components/native-chat/use-native-chat-interactive-send.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-interactive-send.tssrc/renderer/src/components/native-chat/use-native-chat-send-lifecycle.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-send-lifecycle.tssrc/shared/native-chat-streaming.test.ts
|
Addressed the outside-diff CodeRabbit nit in d87459b by memoizing pending-message derivation once and reusing it for streaming preview and session assembly; transcript, marker, streaming-preview, optimistic-send order and pending reconciliation inputs are unchanged. Validation: 8 focused NativeChatView/composer/pending/send-lifecycle/streaming files, 71 tests passed; web typecheck, changed-file oxlint + React Doctor, oxfmt check, git diff check, and max-lines ratchet passed. React Doctor retains five pre-existing state/effect advisories outside this edit; the generic docstring warning is intentionally not actioned because AGENTS.md requires brief why-comments instead of mechanical narration, and there were no inline threads to resolve. @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
Reconcile the desktop native-chat refactor (functions moved to src/shared) with main's in-place improvements: - carry NATIVE_CHAT_ADVANCE_BUFFER_MS 300->500 (stablyai#8568) into the shared answer-stepping module; the mobile stepping module derives from it, so sync mobile-native-chat-answer-stepping.test.ts to 500/1000 - add formatToolInput (stablyai#8654) to the shared tool-summary module and re-export it from the desktop barrel (consumed by NativeChatToolRun) - keep main's settleAfterMs send-handle + useLayoutEffect teardown fix, layered onto the branch's shouldStepNativeChatAskAnswer gating
Missed in merge 8fe3c39, which carried main's NATIVE_CHAT_ADVANCE_BUFFER_MS 300->500 (stablyai#8568) into the shared stepping module that mobile derives from.
…5824) * feat(native-chat): add native chat view across mobile * fix(native-chat): address review findings and CodeRabbit threads Correctness: - Restore an independent initial readSession seed and surface initial-drain errors as snapshot frames so the chat view can never strand on 'loading' - Pair mobile tool results to calls by ordinal FIFO (parallel calls no longer misgraft results); clear a pending ask only when its own call resolves - Show a new streaming reply immediately (same-turn suppression, not length) - Delegate mobile noise filtering to the shared harness-injected classifier - Admit soft-leaving mobile clients in beginMobileInputFloor (parity with mobileTookFloor) so grace-window writes aren't dropped - Self-heal a stale 'working' status once this turn's reply lands - Catch RPC rejections in mobile file-open helpers; guard sanitizeToolInput key collisions; settle web/runtime transports on unrecognized first frames and forward snapshot errors Perf: - Throttle the mobile streaming bubble (50ms) so per-part status frames stop re-parsing the whole accumulated markdown - Short-circuit markdown path detection on dot-less or oversized runs (quadratic backtracking guard) UX/minor: - Wire hold-mode dictation through the native chat composer - Allow scoped-package (@) paths in file-path detection - Move caret after mid-text autocomplete insertion; index-prefixed ask option keys; single scroll-to-end effect; bounded wait + toast when image attach races a resubscribe; count-based pending reconciliation; cache-hit search cancels stale debounce; chat-tab toggle wins over in-flight preference load - Share shouldStepNativeChatAskAnswer between desktop and mobile; import block guards/source priority from shared instead of local copies - Defensive non-positive transcript limits; test strengthening (TTL expiry, post-unsubscribe stale frame, lease readiness, filtered console.error) * refactor(native-chat): share desktop/mobile chat logic in src/shared Extract the parity-mirrored native-chat modules into shared implementations both surfaces re-export: ask parsing (registry, parseAskFromStatus, extractPendingAsk, formatAskAnswer), answer stepping offsets/scheduler, diff detection/parsing, harness-noise filtering, tool fold/pair/split, and tool summaries. Removes the hand-synced copies and their stale Metro comments. Divergence reconciliations take the safer side of each: diffs truncate at 120 lines/32KB everywhere (desktop previously unbounded), tool-run summaries cap at 3 parts with bounded-depth previews, nameless tool calls are skipped, and basenames split on both separators. Also: settle and kill every sibling quick-open pass when one reaches maxResults (main rg/git and relay git; relay rg already did) so a capped search cannot leave a scan walking a huge tree; fold window-bounding into the shared merger's applyAppend; localize the web 'Pair a host' snapshot error. * fix(native-chat): address CodeRabbit follow-ups on shared modules - Attachment lease gate re-checks connection/target/tab after the bounded wait, so a tab/host switch or disconnect mid-wait can't send into a stale terminal; a moved-away target drops silently like the pre-wait guard and only an unrecovered lease surfaces the toast. Adds hook tests. - extractPendingAsk parses transcript tool-calls through the same registered-parser + canonical-shape fallback as live status, so a custom question tool that rendered live survives reconnect/replay. - Direct unit tests for the shared ask parser (FIFO ordering, fallback, malformed payloads) and tool-summary bounded preview (depth/collection caps, circular refs, basename/command branches). * fix(native-chat): treat initialLimit 0 as a valid empty window Both engine guards used truthiness, so an explicit zero limit skipped the bounded tail reader and fell back to an unbounded incremental read. Latent only (every caller clamps positive), hardened for consistency with the tail reader's non-positive-limit handling. * fix(mobile): native-chat composer lock UX + send-failure feedback - Distinguish input-lock reasons: transport 'disconnected' shows Reconnecting… instead of mislabeling a reconnect as locked-by-another-client - Guard the composer lock behind a 600ms hold so connState blips / lease hand-offs don't flicker the placeholder; unlock stays instant - Surface a rejected send inline above the composer (a bottom toast hides behind the keyboard); auto-dismisses after 4s - waiting-session hint invites the first message instead of implying the agent is still starting * test(mobile): sync answer-send pacing test to the 500ms advance buffer Missed in merge 8fe3c39, which carried main's NATIVE_CHAT_ADVANCE_BUFFER_MS 300->500 (#8568) into the shared stepping module that mobile derives from. * fix(mobile): restore terminal stream after chat cold start * fix(native-chat): harden retries, optimistic sends, and file scans * fix(mobile): deliver AskUserQuestion answers by option number (STA-1860) Port #8840's fix to the mobile native chat: the Ask card now tracks per-question option INDICES (+ free text) and the answer-send hook drives Claude's arrow-navigate selector with buildAskAnswerKeys keystroke groups — option numbers, next-tab arrows, Enter — paced one selector step apart, instead of pasting label text that the selector ignores (which silently committed the default option). Non-Claude agents keep the pasted-label path via the selection-based formatAskAnswer. Backcompat: keystrokes are built client-side and written through the EXISTING terminal.send passthrough with enter:false — the same contract the permission card already uses — so an older desktop runtime (SSH/relay included) replays them verbatim; no RPC/contract change in either update order. Free text is newline-sanitized because terminal.send has no paste framing. Drops the now-unused formatCompleteAskAnswer from the shared module. * fix native chat send and runtime races * fix mobile native chat formatting * fix(native-chat): mobile empty state matches desktop copy Mobile showed a single generic line ('Send a message to get started') where desktop shows a titled two-line empty state naming the agent ('Start a chat with Claude' + 'Ask Claude to inspect code, explain output, or make a change.'). Align them from one source of truth so they can't drift again: - Extract the agent-type label map + formatAgentTypeLabel to src/shared/agent-type-label.ts (desktop re-exports; mobile imports). - Add src/shared/native-chat-empty-state.ts with the canonical English copy; desktop uses it as its i18n fallbacks (localization unchanged — en/es/ja/ko/zh keys still win), mobile substitutes the agent label and renders it directly (mobile ships English only). - Mobile: render title + subtitle for waiting-session AND ready-but-empty (both are 'start a chat'), error copy for errors; keep the loading spinner. Live-verified on the iOS sim against a pn-dev of this branch. typecheck node/web + mobile tsc clean; 30 mobile + 428 desktop/shared native-chat tests green. * style: oxfmt the empty-state parity test (line wrap) --------- Co-authored-by: Brennan Benson <brennanbenson@Brennans-MacBook-Pro.local> Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Summary
Related issue: #8401. Canonical first-flush PR: #8418. This is an independent composer-lifecycle follow-up; it does not change transcript reading, polling, transport, or pagination.
Native chat could leave delayed PTY writes and optimistic messages owned by a prior composer target. Repeated prompts could also reconcile against an older identical turn. This PR scopes a send to the active turn by:
Screenshots
No visual change.
Testing
pnpm lint(full lint/custom gates in review; changed-file lint rerun on the rebased head)pnpm typecheck(full typecheck in review; web typecheck rerun on the rebased head)pnpm testwas not run; 48 focused files / 408 tests passed in review, then the exact rebased 7-file suite passed 67/67pnpm buildwas not runAI Review Report
Two separate
gpt-5.6-solxhigh agents implemented and adversarially reviewedorigin/main..HEAD. The second pass found and fixed five lifecycle gaps: Stop bypassing owned-send cancellation, interactive answers surviving PTY rebinding, cancelled sends lingering in the optimistic cache, attachment-only sends not reconciling, and renderer-clock fallback after an authoritative boundary was paged out.The review covered macOS, Linux, and Windows input behavior; local and SSH/runtime PTY ownership; delayed timer cleanup; target changes and remounts; repeated/identical prompts; bounded history; image paths; performance; and overlap with #8418, #8157, and #8288. No platform-specific shortcut, path separator, shell behavior, or Electron branch was introduced.
Security Audit
No new command execution surface, IPC/RPC method, dependency, secret, auth, or credential handling. User text and image paths continue through the existing terminal-input path; this change reduces the period in which queued input can outlive its owning pane. Pending state remains renderer-local and bounded by the existing cache/pruning rules.
Notes