Repository navigation
fix(codex): keep async questions available across desktop and mobile - #30062
miguel93041 wants to merge 11 commits into
Conversation
miguel93041
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 53.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 126 functions across 78 files. (6 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the primary change: preserving Codex async questions across desktop and mobile. Description check The description follows the required template and explains the user impact, implementation, rationale, linked issues, visual proof, testing, AI use, review notes, compatibility considerations, and che… Linked Issues check The PR meets the coding requirements in [ #20073] and [#22485]. Shared parsing recognizes confirmedrequest_user_input_asyncdata, including title-only questions and options. Desktop and mobile rende…Out of Scope Changes check The changes remain within [ #20073] and [#22485]. The host tracking, transport fields, desktop and mobile cards, reply delivery, localization, and regression tests support async-question cards and thei…Full details: Description check
-
Explanation
The description follows the required template and explains the user impact, implementation, rationale, linked issues, visual proof, testing, AI use, review notes, compatibility considerations, and checklist status. It also clearly records incomplete full-suite, platform, and lint validation.
Full details: Linked Issues check
-
Explanation
The PR meets the coding requirements in [
#20073] and [#22485]. Shared parsing recognizes confirmedrequest_user_input_asyncdata, including title-only questions and options. Desktop and mobile render selectable answers and free-textOtheranswers without using blocking TUI input. Host and transcript state keep questions pending after acknowledgement, turn completion, long output, and chat reopening. Delivered messages resolve the correct questions, while failed or unconfirmed sends do not. Tests cover parsing, rendering, delivery, reconstruction, resolution, and reopening.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within [
#20073] and [#22485]. The host tracking, transport fields, desktop and mobile cards, reply delivery, localization, and regression tests support async-question cards and their pending state. The reviewed changes do not add unrelated plan-approval, shortcut, orchestration, or provider-RPC work.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 53.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 126 functions across 78 files. (6 skipped: 6 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d12f8d12-b360-4677-a837-d82d487276b0
📒 Files selected for processing: 22
src/main/codex/codex-async-question-block.tssrc/main/codex/codex-async-questions.test.tssrc/main/codex/codex-structured-item-translation.tssrc/main/codex/codex-structured-prompt-items.test.tssrc/main/codex/codex-structured-prompt-items.tssrc/renderer/src/components/native-chat/NativeChatAssistantMessageRow.tsxsrc/renderer/src/components/native-chat/NativeChatAsyncQuestions.test.tsxsrc/renderer/src/components/native-chat/NativeChatAsyncQuestions.tsxsrc/renderer/src/components/native-chat/NativeChatMessageRow.async-questions.test.tsxsrc/renderer/src/components/native-chat/NativeChatQuestionCard.tsxsrc/renderer/src/components/native-chat/NativeChatResolvedView.prompt-card-input.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredMessageProviders.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredMessageProviders.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredQuestionCard.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.tsxsrc/renderer/src/components/native-chat/native-chat-async-question-context.tssrc/renderer/src/components/native-chat/use-structured-agent-session-sends.test.tsxsrc/renderer/src/components/native-chat/use-structured-agent-session-sends.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/shared/agent-session-journal-schemas.tssrc/shared/native-chat-types.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
miguel93041
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
448f9a34-c191-41f8-8636-1257d7c31b4a
📒 Files selected for processing: 23
mobile/src/session/MobileNativeChatAsk.tsxmobile/src/session/MobileNativeChatAsyncQuestionProvider.tsxmobile/src/session/MobileNativeChatAsyncQuestions.tsxmobile/src/session/MobileNativeChatMessage.tsxmobile/src/session/MobileNativeChatOverlay.tsxmobile/src/session/mobile-native-chat-async-question-context.tsmobile/src/session/mobile-native-chat-controller-contract.tsmobile/src/session/mobile-native-chat-structured-presentation.tsmobile/src/session/mobile-structured-agent-session-contract.tsmobile/src/session/use-mobile-native-chat-async-reply.tsmobile/src/session/use-mobile-native-chat-controller.tsmobile/src/session/use-mobile-native-chat-message-send.tsmobile/src/session/use-mobile-structured-send-with-outcome.tssrc/main/codex/codex-async-question-block.tssrc/main/codex/codex-structured-item-translation.tssrc/main/native-chat/transcript-line-decoders-codex.tssrc/renderer/src/components/native-chat/NativeChatAsyncQuestions.tsxsrc/renderer/src/components/native-chat/NativeChatResolvedView.tsxsrc/renderer/src/components/native-chat/native-chat-async-question-context.tssrc/renderer/src/components/native-chat/use-native-chat-terminal-async-reply.tssrc/renderer/src/components/native-chat/use-native-chat-working-suppression.tssrc/shared/native-chat-ask.tssrc/shared/native-chat-async-questions.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| const cancel = useCallback(() => { | ||
| const current = pending.current | ||
| if (!current) { | ||
| return | ||
| } | ||
| // A new blocking prompt may now own the input: cancel delayed writes without clearing it. | ||
| current.handle?.cancel(true) | ||
| current.finish(false) | ||
| }, []) | ||
| useLayoutEffect(() => { | ||
| latest.current = args | ||
| }) | ||
| useLayoutEffect(() => { | ||
| mounted.current = true | ||
| return () => { | ||
| mounted.current = false | ||
| cancel() | ||
| } | ||
| }, [cancel, paneKey, sessionId, targetPtyId, runtimeEnvironmentId]) | ||
| useLayoutEffect(() => { | ||
| if (disabled) { | ||
| cancel() | ||
| } | ||
| }, [cancel, disabled]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,120p' src/renderer/src/components/native-chat/use-native-chat-terminal-async-reply.ts
rg -n 'useNativeChatTerminalAsyncReply|canSend\b|isVisible|readPhase' src/renderer/src/components/native-chat/NativeChatResolvedView.tsx | head -40
rg -n 'export function sendNativeChatMessage|cancel\(|setTimeout|submitDelay' src/renderer/src/components/native-chat/*.ts | head -40
rg -n "readPhase" src/renderer/src/components/native-chat --type=ts -g '!*.test.*' | head -40Repository: stablyai/orca
Length of output: 13259
🏁 Script executed:
set -eu
printf '%s\n' '--- NativeChatResolvedView hook call ---'
sed -n '60,135p' src/renderer/src/components/native-chat/NativeChatResolvedView.tsx
sed -n '200,255p' src/renderer/src/components/native-chat/NativeChatResolvedView.tsx
printf '%s\n' '--- native-chat-runtime-send.ts ---'
sed -n '1,180p' src/renderer/src/components/native-chat/native-chat-runtime-send.ts
printf '%s\n' '--- live session read-phase update ---'
sed -n '300,375p' src/renderer/src/components/native-chat/use-native-chat-live-session.ts
printf '%s\n' '--- structured live session phase selection ---'
sed -n '1,90p' src/renderer/src/components/native-chat/use-structured-chat-live-session.ts
printf '%s\n' '--- retained session phase transitions ---'
sed -n '1,75p' src/renderer/src/components/native-chat/use-native-chat-retained-session.ts
printf '%s\n' '--- pending delivery settlement ---'
sed -n '1,210p' src/renderer/src/components/native-chat/use-native-chat-pending-delivery.tsRepository: stablyai/orca
Length of output: 25821
🏁 Script executed:
set -eu
printf '%s\n' '--- observed terminal writes ---'
sed -n '1,260p' src/renderer/src/components/native-chat/native-chat-observed-send.ts
printf '%s\n' '--- live-session read declaration and read usage ---'
rg -n -C4 'const read|read =|use.*Read|phase:|setRead|read\.' src/renderer/src/components/native-chat/use-native-chat-live-session.ts
sed -n '1,180p' src/renderer/src/components/native-chat/use-native-chat-live-session.ts
printf '%s\n' '--- NativeChatResolvedView callers and visibility source ---'
rg -n -C3 'NativeChatResolvedView|isVisible=' src/renderer/src/components/native-chat src/renderer/src | head -120Repository: stablyai/orca
Length of output: 30730
🏁 Script executed:
set -eu
printf '%s\n' '--- input clear contract ---'
rg -n -C6 'export type NativeChatSendOptions|function clearUnsubmittedAgentInput|const clearUnsubmittedAgentInput|clearUnsubmittedAgentInput' src/renderer/src/components/native-chat/native-chat-input-clear.ts
sed -n '1,240p' src/renderer/src/components/native-chat/native-chat-input-clear.ts
printf '%s\n' '--- PTY send queue cancellation ---'
sed -n '1,300p' src/renderer/src/components/native-chat/native-chat-pty-send-queue.tsRepository: stablyai/orca
Length of output: 10782
🏁 Script executed:
set -eu
printf '%s\n' '--- decisive line-numbered excerpts ---'
nl -ba src/renderer/src/components/native-chat/use-native-chat-terminal-async-reply.ts | sed -n '20,115p'
nl -ba src/renderer/src/components/native-chat/native-chat-runtime-send.ts | sed -n '45,85p'
nl -ba src/renderer/src/components/native-chat/native-chat-observed-send.ts | sed -n '15,90p'
nl -ba src/renderer/src/components/native-chat/native-chat-pty-send-queue.ts | sed -n '110,205p'
nl -ba src/renderer/src/components/native-chat/native-chat-input-clear.ts | sed -n '19,45p'
nl -ba src/renderer/src/components/native-chat/NativeChatResolvedView.tsx | sed -n '88,110p;220,240p'
nl -ba src/renderer/src/components/native-chat/use-native-chat-live-session.ts | sed -n '118,165p;210,250p;320,355p'
nl -ba src/renderer/src/components/native-chat/use-native-chat-pending-delivery.ts | sed -n '80,145p'
printf '%s\n' '--- owning render references ---'
rg -n -F --glob '*.tsx' -- 'NativeChatResolvedView' src || test "$?" -eq 1Repository: stablyai/orca
Length of output: 26643
🏁 Script executed:
set -eu
nl -ba src/renderer/src/components/native-chat/NativeChatView.tsx | sed -n '1,95p'
nl -ba src/renderer/src/components/native-chat/native-chat-view-types.ts | sed -n '1,75p'Repository: stablyai/orca
Length of output: 6618
Restrict cancellation to input-ownership changes.
When the user submits an answer, sendNativeChatMessage writes the paste and schedules Enter as a separate delayed PTY write. If isVisible becomes false while that write is pending, the current effect calls cancel(). cancel(true) suppresses the delayed Enter but preserves the pasted input. finish(false) then holds the delivery as unconfirmed, so the submitted answer remains an unsubmitted TUI draft.
Keep disabled for blocking new sends, but use a separate cancellation condition for promptCard !== null || !canSend.
🐛 Suggested fix
type PendingReply = { handle: NativeChatSendHandle | null; finish: (accepted: boolean) => void }
export function useNativeChatTerminalAsyncReply(args: {
paneKey: string
sessionId: string | null
targetPtyId: string | null
runtimeEnvironmentId: string | null
disabled: boolean
+ cancelInFlight: boolean
delivery: ReturnType<typeof useNativeChatPendingDelivery>
onSubmitted: () => void
}) {
- const { paneKey, sessionId, targetPtyId, runtimeEnvironmentId, disabled } = args
+ const { paneKey, sessionId, targetPtyId, runtimeEnvironmentId, disabled } = args
...
useLayoutEffect(() => {
- if (disabled) {
+ if (args.cancelInFlight) {
cancel()
}
- }, [cancel, disabled])
+ }, [args.cancelInFlight, cancel]) runtimeEnvironmentId,
disabled: !canSend || !isVisible || promptCard !== null || session.readPhase !== 'ready',
+ cancelInFlight: !canSend || promptCard !== null,
delivery,
|
@coderabbitai resume |
✅ Action performedReviews resumed. No new commits to review. |
Derive pending questions from delivered history so cards remain available outside the loaded message window. Extend the current-request transport from stablyai#29121 with opt-in async rows and adapt rollout reconstruction and regressions from stablyai#25448.
miguel93041
changed the title
|
This PR carries forward the Codex async-question work from #25448 and adapts it to the current request transport merged in #29121. It covers desktop and mobile, structured and terminal-backed chats, and pending questions outside the loaded history, with resolution shared across clients. It does not replace all of #25448. That proposal also unified how desktop and mobile choose question and approval cards, and addressed mobile misreading numbered Claude plan steps as terminal menu choices. Those changes are not included here; this PR does not claim to fix #24415. Once reviewed and merged, this could serve as a new base for resuming that remaining work. The async-question handling would already be in place, leaving the shared card-selection logic and plan-approval fixes to a follow-up. The terminal plan menus would still need to be checked against captured terminal output, as #25448 noted. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
src/main/ipc/native-chat.ts: 1
233-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
asyncQuestionsto theNativeChatAppendedPayloadframe types.These frames now carry
asyncQuestions, but the payload type at Lines 65-89 does not declare it. The code compiles only because TypeScript skips excess-property checks for spread properties. The renderer reads the field throughNativeChatSubscriptionFramein preload. If the main-process type does not declare it, a later refactor can drop the field without a compile error.♻️ Proposed type update
// in each of the snapshot / replacement / appended variants asyncQuestions?: NativeChatAsyncQuestionsField
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d52fff6b-faab-4112-8378-7345c447d489
📒 Files selected for processing: 62
mobile/src/session/MobileNativeChatAsyncQuestionProvider.tsxmobile/src/session/MobileNativeChatAsyncQuestions.test.tsxmobile/src/session/MobileNativeChatMessage.tsxmobile/src/session/MobileNativeChatOverlay.tsxmobile/src/session/MobileNativeChatPendingAsyncQuestions.tsxmobile/src/session/MobileNativeChatView.tsxmobile/src/session/mobile-native-chat-stream-frame.tsmobile/src/session/use-mobile-native-chat-session.tsmobile/src/session/use-mobile-structured-agent-session.tssrc/main/codex/codex-async-question-block.tssrc/main/codex/codex-async-questions.test.tssrc/main/codex/codex-structured-item-translation.tssrc/main/ipc/native-chat.tssrc/main/native-chat/agent-session-wire/agent-session-current-requests.tssrc/main/native-chat/agent-session-wire/agent-session-pending-async-questions.tssrc/main/native-chat/agent-session-wire/structured-agent-session-pending-async.test.tssrc/main/native-chat/codex-async-question-reply.test.tssrc/main/native-chat/codex-rollout-async-question-facts.tssrc/main/native-chat/transcript-async-question-boundary-scan.test.tssrc/main/native-chat/transcript-async-question-boundary-scan.tssrc/main/native-chat/transcript-async-question-fold-cache.tssrc/main/native-chat/transcript-async-question-retry.test.tssrc/main/native-chat/transcript-async-question-tracker.test.tssrc/main/native-chat/transcript-async-question-tracker.tssrc/main/native-chat/transcript-incremental-reader.tssrc/main/native-chat/transcript-line-decoders-codex.tssrc/main/native-chat/transcript-reader-codex-async-questions.test.tssrc/main/native-chat/transcript-watch-contract.tssrc/main/native-chat/transcript-watch-engine.tssrc/main/runtime/rpc/methods/native-chat.tssrc/preload/api/native-chat-api.tssrc/renderer/src/components/native-chat/NativeChatAssistantMessageRow.tsxsrc/renderer/src/components/native-chat/NativeChatAsyncQuestions.test.tsxsrc/renderer/src/components/native-chat/NativeChatPendingAsyncQuestions.tsxsrc/renderer/src/components/native-chat/NativeChatResolvedView.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredMessageProviders.tsxsrc/renderer/src/components/native-chat/native-chat-live-session-contract.tssrc/renderer/src/components/native-chat/native-chat-session-transport.test.tssrc/renderer/src/components/native-chat/native-chat-session-transport.tssrc/renderer/src/components/native-chat/native-chat-stream-teardown.tssrc/renderer/src/components/native-chat/use-native-chat-live-session.tssrc/renderer/src/components/native-chat/use-native-chat-pending-async-questions.tssrc/renderer/src/components/native-chat/use-native-chat-terminal-async-reply.tssrc/renderer/src/components/native-chat/use-structured-agent-session.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/web/preload-api/web-native-chat-api.tssrc/shared/agent-session-current-requests-reader.tssrc/shared/agent-session-current-requests-wire.tssrc/shared/agent-session-journal-schemas.tssrc/shared/codex-async-question-item.tssrc/shared/codex-async-question-reply.tssrc/shared/native-chat-async-question-facts.tssrc/shared/native-chat-async-questions.tssrc/shared/native-chat-pending-async-questions.test.tssrc/shared/native-chat-pending-async-questions.tssrc/shared/native-chat-types.tssrc/shared/rpc-contract/agent-session-current-requests-params.ts
🚧 Files skipped from review as they are similar to previous changes: 6
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/i18n/locales/ja.json
- src/renderer/src/i18n/locales/fr.json
- src/renderer/src/i18n/locales/ko.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
ELI5
Codex can ask a question while it keeps working. Orca now shows a card with suggested answers and an Other field on desktop and mobile. Pending cards stay available after long output or reopening the chat, even when the original message is not loaded. Once the host records a delivered reply, the other clients remove the resolved questions too.
What Changed
Both structured chat and terminal-backed chat use the existing question controls and message senders. Structured replies reach Codex immediately while it works. Blocking questions also respect the question-level
isOtherflag.The execution host derives pending async questions from delivered history. Structured chat uses the existing
agentSession.currentRequeststransport; terminal chat reconstructs the set from the rollout and updates it as records arrive. Clients show those cards near the composer, independently of the loaded message window. Ending a turn does not resolve them.A delivered ordinary message retires the preceding questions, matching the rule in #25448. Replies from Codex’s question editor retire only the questions they identify. Queued or unconfirmed messages do not resolve the shared set. Folding a card preserves its draft; new cards do not steal focus. Desktop labels cover all six built-in languages.
Why
The host has the complete history; a client may have only its latest page. Deriving pending questions there keeps devices consistent without adding a second persisted store. Async questions remain separate from blocking approvals, so they survive a turn ending.
Linked Issue
Fixes #20073 — missing async question cards on mobile.
Fixes #22485 — missing async question cards in desktop chat.
Earlier proposals, oldest first:
This PR extends the current-request transport merged in #29121 and adapts pending-question reconstruction and regression tests from #25448. It does not include that proposal’s plan-approval changes.
Related reports with different scope: #21247 and its closed PR #21249 concern Answer/Skip shortcuts in the mobile terminal; #24415 concerns plan approvals; #25946 concerns orchestration nudges entering Codex's question editor. This PR does not claim to fix those reports.
Visual Proof
Desktop before and after, captured with production components and synthetic input in a hidden renderer:
Mobile pending card with Other selected, rendered with React Native Web. The scroll and rejected-send paths were exercised; this is not a phone or live delivery check.
Testing
384 focused tests pass across 37 suites: 279 desktop/host tests with the default
pnpm testrunner and Bun 1.4.3, plus 105 mobile tests. Coverage includes two clients reading a question outside their loaded history, unconfirmed delivery, resolution and reopening, partial question-editor replies, transcript replacement, context clears followed by a late send rejection, reconnects, WSL reads, cancelled subscriptions, draft preservation and Other answers.Desktop and mobile typechecks, changed-code quality, mobile lint, localization and RPC catalog checks pass.
pnpm buildpasses, including desktop, web and mobile-web assets. iOS and Android JavaScript bundle exports also pass. A native Android arm64 debug APK built during the earlier iteration; it has not been rebuilt for this update. No native iOS build or physical-device run was performed.A real async question and Other reply were verified in an earlier macOS build. The latest desktop and mobile components were rendered without errors in hidden windows. Multi-client and transport tests use simulated clients; live SSH, Windows, Linux and physical phones remain untested.
Full lint previously stopped on five import-cycle warnings in unchanged mobile transport and terminal files. The full test suite has not run. PR Checks and Mobile Checks require maintainer approval for this fork, so the full-CI checklist remains unchecked.
Manual check: ask Codex for an async question, let it produce enough output to move the message out of the loaded window, then reopen the chat on another client. Both should show the pending card. Answer with Other and verify it disappears on both after delivery. Repeat with a failed send and check the question remains available.
AI Disclosure
Implementation, tests and review assisted by OpenAI Codex (GPT-6).
Review
Reviewed reply routing, partial answers, input ownership, reconnects and delivery failures. The earlier CodeRabbit findings remain covered: hiding a view does not cancel an admitted reply, and streamed updates do not reset the reply context. The final review also caught a late send rejection hiding newer questions after a context clear. Submission lookup now retains the original delivery outcome; a regression test fails before the fix and passes after it. Uncertain sends are not retried automatically.
No new dependencies or RPC methods. New hosts advertise async support and include the extra rows only when requested; older clients retain the previous response shape. New clients connected to older hosts keep inline cards. Metadata and stream fields are optional. No migration is needed.
Agent skill upstream boundary
Notes
Mobile reuses its existing English-only controls. Plugin language packs retain their English fallback. Folded cards and drafts are local to each view; pending-question resolution comes from the host. This does not guarantee deduplication if two users send answers at the same time.
Terminal reconstruction caches completed reads and limits published question data to 256 KiB, with a notice when more questions remain. Structured chat uses its existing chunked reads.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)