Repository navigation
Recover native chat messages when terminal delivery is unconfirmed - #12643
brennanb2025 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds prompt-submission metadata across agent hooks, relay transport, pane status history, IPC, and runtime projections. Renderer and mobile native chat now track optimistic sends, match prompt acknowledgements, restore unconfirmed messages after timeout, and expose delivery failure state. Both platforms render recovery notices that preserve the message and provide a terminal-navigation action. Tests cover digest generation, history bounds, acknowledgement ordering, send submission, timeout recovery, and recovery UI behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/src/session/mobile-native-chat-send.ts (1)
119-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the interactive-prompt refusal through draft clearing.
When the guarded clear receives
refusedReason: 'interactive-prompt', this function converts it tofalse. The caller then restores the draft but reports"Message not sent"instead of the terminal-prompt recovery message.Return a typed outcome or refusal reason from
clearMobileNativeChatInput. Handleinteractive-promptlike the body-send refusal. Add a seeded-launch-draft regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e502315-0a0a-45ee-a8a6-d37b1f47da60
📒 Files selected for processing (16)
mobile/src/session/mobile-native-chat-image-send.test.tsmobile/src/session/mobile-native-chat-image-send.tsmobile/src/session/mobile-native-chat-send.test.tsmobile/src/session/mobile-native-chat-send.tsmobile/src/session/use-mobile-native-chat-message-send.test.tsmobile/src/session/use-mobile-native-chat-message-send.tsmobile/src/terminal/terminal-send-rpc-response.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/terminal-send.test.tssrc/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/native-chat-terminal-prompt-guard.tssrc/renderer/src/i18n/locales/en.jsonsrc/shared/runtime-types.tssrc/shared/terminal-wait-blocked-detection.ts
Electron QAValidated at
Mobile QAThe PR mobile app loaded in the iOS simulator, and the focused mobile suite passed (49 tests). Live pairing to the isolated PR runtime was interrupted by a host daemon-version restart, so the target mobile workflow was not claimed as end-to-end validated. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
brennanb2025
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
mobile/src/session/use-mobile-native-chat-delivery-recovery.test.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the deprecated
react-test-rendererwarning guard.This hook test only renders a null
Harness, so keep the existing state-based assertions and replace theact/create/update/unmountlifecycle with the supported Testing Library harness. Move the relatedreact-test-rendererdependency cleanup into the broader mobile test migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9baef488-a26f-4501-ac40-64fe7b89489d
📒 Files selected for processing (50)
mobile/src/session/MobileNativeChatDeliveryRecoveryNotice.tsxmobile/src/session/MobileNativeChatOverlay.tsxmobile/src/session/MobileNativeChatView.test.tsmobile/src/session/MobileNativeChatView.tsxmobile/src/session/mobile-native-chat-controller-contract.tsmobile/src/session/mobile-native-chat-pending-echo.tsmobile/src/session/mobile-native-chat-prompt-delivery.test.tsmobile/src/session/mobile-native-chat-prompt-delivery.tsmobile/src/session/mobile-native-chat-view-styles.tsmobile/src/session/use-mobile-native-chat-controller.tsmobile/src/session/use-mobile-native-chat-delivery-recovery.test.tsmobile/src/session/use-mobile-native-chat-delivery-recovery.tsmobile/src/session/use-mobile-native-chat-drafts.tssrc/main/agent-hooks/hook-status-session-tabs-invalidation.test.tssrc/main/agent-hooks/hook-status-session-tabs-invalidation.tssrc/main/agent-hooks/server.test.tssrc/main/agent-hooks/server.tssrc/main/index.tssrc/main/runtime/orca-runtime-hook-agent-status-projection.test.tssrc/main/runtime/orca-runtime.tssrc/main/ssh/ssh-relay-session.tssrc/relay/agent-hook-server.test.tssrc/relay/agent-hook-server.tssrc/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/NativeChatDeliveryRecoveryNotice.test.tsxsrc/renderer/src/components/native-chat/NativeChatDeliveryRecoveryNotice.tsxsrc/renderer/src/components/native-chat/NativeChatView.tsxsrc/renderer/src/components/native-chat/native-chat-composer-types.tssrc/renderer/src/components/native-chat/native-chat-pending.tssrc/renderer/src/components/native-chat/native-chat-prompt-delivery.test.tssrc/renderer/src/components/native-chat/native-chat-prompt-delivery.tssrc/renderer/src/components/native-chat/native-chat-pty-send-queue.tssrc/renderer/src/components/native-chat/native-chat-runtime-send.tssrc/renderer/src/components/native-chat/use-native-chat-composer-attachments.tssrc/renderer/src/components/native-chat/use-native-chat-composer-target.tssrc/renderer/src/components/native-chat/use-native-chat-delivery-recovery.tssrc/renderer/src/components/native-chat/use-native-chat-pending-delivery.tssrc/renderer/src/components/native-chat/use-native-chat-send-lifecycle.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-send-lifecycle.tssrc/renderer/src/hooks/useIpcEvents.test.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/store/slices/agent-prompt-submission-history.tssrc/renderer/src/store/slices/agent-status-prompt-submission.test.tssrc/renderer/src/store/slices/agent-status.tssrc/shared/agent-hook-listener.test.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-relay.tssrc/shared/agent-status-types.ts
| const check: MobileNativeChatDeliveryCheck = { | ||
| pendingId: 'pending-1', | ||
| draftKey: 'draft', | ||
| pendingKey: 'pending', | ||
| text: 'hello', | ||
| normalizedText: 'hello', | ||
| baselineTailMessageId: null, | ||
| submittedAt: 20, | ||
| expectedDigest: mobileNativeChatPromptDigest('hello'), | ||
| baseline: { streamId: 'stream', sequence: 2, digest: 'old', receivedAt: 10 }, | ||
| deadline: null | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 3 "submittedAt|export type MobileNativeChatDeliveryCheck" mobile/src/session/mobile-native-chat-prompt-delivery.ts mobile/src/session/mobile-native-chat-prompt-delivery.test.tsRepository: stablyai/orca
Length of output: 2849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Type definition"
sed -n '1,45p' mobile/src/session/mobile-native-chat-prompt-delivery.ts
echo
echo "## Test imports and type usages"
sed -n '1,70p' mobile/src/session/mobile-native-chat-prompt-delivery.test.ts
echo
sed -n '70,105p' mobile/src/session/mobile-native-chat-prompt-delivery.test.ts
echo
echo "## Check whether submittedAt is read/required anywhere"
rg -n "submittedAt|MobileNativeChatDeliveryCheck" mobile/src/session/mobile-native-chat-prompt-delivery.ts mobile/src/session/mobile-native-chat-prompt-delivery.test.tsRepository: stablyai/orca
Length of output: 5794
Remove the stale submittedAt properties.
MobileNativeChatDeliveryCheck does not define submittedAt, and the implementation does not read it. Keep the named type literals clean at lines 26, 51, and 87.
| export function NativeChatDeliveryRecoveryNotice({ | ||
| onShowTerminal | ||
| }: { | ||
| onShowTerminal?: () => void | ||
| }): React.JSX.Element { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hide the terminal action when navigation is unavailable.
onShowTerminal is optional on Lines 5-9. Line 27 always renders an enabled button. If a caller omits the callback, “Show terminal” does nothing.
Render the button only when onShowTerminal exists. Alternatively, make onShowTerminal required and update all callers. Add a test for the callback-absent case.
Proposed fix
- <Button type="button" variant="secondary" size="sm" onClick={onShowTerminal}>
- {translate('components.native-chat.toggle.showTerminal', 'Show terminal')}
- </Button>
+ {onShowTerminal ? (
+ <Button type="button" variant="secondary" size="sm" onClick={onShowTerminal}>
+ {translate('components.native-chat.toggle.showTerminal', 'Show terminal')}
+ </Button>
+ ) : null}Also applies to: 27-29
|
Final-head review ( The same-model senior pass found one meaningful issue and fixed it: image-bearing sends no longer enter the text-only digest timeout, which could falsely restore an already delivered image message and invite a duplicate retry. The follow-up review returned clean. Validation:
Performance remains bounded: eight-entry confirmation history/cache, one owned timeout only for confirmable text sends, cleanup on acknowledgement/unmount, and no polling or subprocess loop. Desktop recovery: Terminal confirmation: |
|
Closing this terminal-delivery recovery approach. Current chat sends use the host’s send result, and uncertain terminal delivery is already reported by #23788 without claiming the message was never sent. |
1 participant
Summary
UserPromptSubmitacknowledgements, with transcript fallback, instead of inferring terminal UI state or matching provider-specific stringsThe recovery UI says:
Validation
CLEANgit diff --checkpassedElectron evidence
Unconfirmed message restored in native chat
Show terminal reveals the pending terminal interaction