feat: Queue vs Steer follow-up parity with intent shortcuts#467
feat: Queue vs Steer follow-up parity with intent shortcuts#467amanthanvi wants to merge 8 commits intoDimillian:mainfrom
Conversation
|
Validation update:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87ff8aa387
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| markProcessing(threadId, false); | ||
| setActiveTurnId(threadId, null); |
There was a problem hiding this comment.
This branch now clears isProcessing/activeTurnId for every turn/steer error, but steer failures can also be transient transport or RPC failures while the original turn is still running. In that case, returning steer_failed here causes useQueuedSend to enqueue the fallback message and then immediately flush it once processing is marked false, so follow-ups can be sent out of order (or collide with the still-active turn). Please only clear processing state for confirmed stale-turn errors (for example, "no active turn"), and keep the thread marked processing for other steer failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in ee34c3a.
Adjusted steer failure handling in useThreadMessaging to preserve isProcessing / activeTurnId on steer RPC/transport failures, while still returning steer_failed so the payload can be queued for retry.
There was a problem hiding this comment.
Pull request overview
Implements Codex-style parity for follow-up messaging while a turn is processing by introducing an explicit default follow-up behavior (queue | steer), an opposite-action shortcut, and end-to-end “send intent” routing (settings → composer → messaging hooks), including steer-failure recovery that clears stale state and queues the follow-up.
Changes:
- Add persisted
followUpMessageBehavior(with TS + Rust migration from legacysteerEnabled) and a Settings → Composer segmented control. - Update composer UX (hint copy + send button labeling) and keyboard behavior (add Shift+Cmd/Ctrl+Enter opposite action; remove legacy Tab-to-queue).
- Route explicit send intents through the thread messaging stack and handle steer failure by clearing state and queueing the payload; update tests + docs accordingly.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Introduces FollowUpMessageBehavior / ComposerSendIntent types and persists follow-up behavior in AppSettings. |
| src/styles/settings.css | Adds segmented-control styling for the new Composer settings control. |
| src/styles/composer.css | Adds styling for the in-composer follow-up behavior hint. |
| src/features/workspaces/hooks/useWorkspaceHome.ts | Broadens messaging callback return type to accommodate new send result shape. |
| src/features/threads/hooks/useThreads.integration.test.tsx | Updates integration test fixture to include followUpMessageBehavior. |
| src/features/threads/hooks/useThreadMessaging.ts | Adds sendIntent plumbing + structured send result and steer-failure handling. |
| src/features/threads/hooks/useThreadMessaging.test.tsx | Adds coverage for steer-failure return values and stale-state clearing. |
| src/features/threads/hooks/useQueuedSend.ts | Computes effective intent (default vs explicit queue/steer) and queues on steer failure. |
| src/features/threads/hooks/useQueuedSend.test.tsx | Updates tests for new intent routing + steer-failure queue fallback. |
| src/features/settings/hooks/useSettingsViewOrchestration.ts | Provides platform-specific opposite-action shortcut label to settings UI. |
| src/features/settings/hooks/useSettingsFeaturesSection.ts | Hides legacy steer dynamic feature row and adjusts feature filtering. |
| src/features/settings/hooks/useAppSettings.ts | Adds defaults + normalization for followUpMessageBehavior with legacy fallback. |
| src/features/settings/components/sections/SettingsFeaturesSection.tsx | Updates steer feature description text to reflect capability (not follow-up mode). |
| src/features/settings/components/sections/SettingsComposerSection.tsx | Adds Settings → Composer segmented control and help copy for follow-up behavior. |
| src/features/settings/components/SettingsView.test.tsx | Adds tests for composer follow-up setting and hides legacy steer row. |
| src/features/layout/hooks/layoutNodes/types.ts | Extends composer callback types with submitIntent; replaces steerEnabled with steerAvailable + follow-up behavior wiring. |
| src/features/layout/hooks/layoutNodes/buildPrimaryNodes.tsx | Wires steerAvailable and follow-up behavior into Composer; simplifies send label handling. |
| src/features/git/hooks/usePullRequestReviewActions.ts | Broadens messaging callback return type to accommodate new send result shape. |
| src/features/git/hooks/usePullRequestComposer.ts | Plumbs submitIntent through PR composer send handling. |
| src/features/git/hooks/usePullRequestComposer.test.tsx | Updates test expectations for the expanded handleSend signature. |
| src/features/composer/components/ComposerSend.test.tsx | Adds keyboard behavior matrix coverage (processing vs non-processing; default vs opposite intent; Tab removal). |
| src/features/composer/components/ComposerInput.tsx | Adds title attribute to match send/stop labeling behavior. |
| src/features/composer/components/ComposerEditorHelpers.test.tsx | Updates harness props for new Composer inputs (steer availability + follow-up behavior). |
| src/features/composer/components/Composer.tsx | Implements shortcut handling + in-composer hint + effective label + submit-intent routing; removes Tab-to-queue path. |
| src/features/app/orchestration/useThreadOrchestration.ts | Propagates submitIntent through orchestration handlers. |
| src/features/app/hooks/usePlanReadyActions.ts | Improves collaboration-mode selection persistence + adjusts send options typing. |
| src/features/app/hooks/usePlanReadyActions.test.tsx | Adds unit coverage for collaboration-mode selection/persistence behavior. |
| src/features/app/hooks/useComposerController.ts | Plumbs follow-up behavior into queued-send controller and updates send signatures. |
| src/App.tsx | Computes steerAvailable and wires follow-up behavior into layout/composer controller; plumbs persistThreadCodexParams into plan-ready actions. |
| src-tauri/src/types.rs | Adds persisted followUpMessageBehavior to Rust settings model + default. |
| src-tauri/src/storage.rs | Migrates legacy steerEnabled into followUpMessageBehavior during settings read; adds tests. |
| docs/app-server-events.md | Updates documentation to reflect steer behavior and steer-failure queuing semantics. |
| README.md | Updates user-facing docs for follow-up behavior and opposite-action shortcuts. |
| AGENTS.md | Adds an ownership/map section for follow-up behavior implementation areas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="settings-field"> | ||
| <div className="settings-field-label">Follow-up behavior</div> | ||
| <div className="settings-segmented" role="radiogroup" aria-label="Follow-up behavior"> | ||
| <button | ||
| type="button" | ||
| role="radio" | ||
| aria-checked={appSettings.followUpMessageBehavior === "queue"} | ||
| className={`settings-segmented-option${ | ||
| appSettings.followUpMessageBehavior === "queue" ? " is-active" : "" | ||
| }`} | ||
| onClick={() => | ||
| void onUpdateAppSettings({ | ||
| ...appSettings, | ||
| followUpMessageBehavior: "queue", | ||
| }) | ||
| } | ||
| > | ||
| Queue | ||
| </button> | ||
| <button | ||
| type="button" | ||
| role="radio" | ||
| aria-checked={appSettings.followUpMessageBehavior === "steer"} | ||
| className={`settings-segmented-option${ | ||
| appSettings.followUpMessageBehavior === "steer" ? " is-active" : "" | ||
| }`} | ||
| disabled={steerUnavailable} | ||
| title={steerUnavailable ? "Steer is unavailable in the current Codex config." : ""} | ||
| onClick={() => | ||
| void onUpdateAppSettings({ | ||
| ...appSettings, | ||
| followUpMessageBehavior: "steer", | ||
| }) | ||
| } | ||
| > |
There was a problem hiding this comment.
This segmented control uses role="radiogroup"/role="radio" on <button> elements but doesn’t implement expected radio-group keyboard behavior (roving tabIndex, arrow-key navigation, etc.). For accessibility, either use native <input type="radio"> elements or add the required focus/keyboard management for ARIA radios.
There was a problem hiding this comment.
Implemented in 190a5fc.
Replaced the segmented pseudo-radios with native <input type="radio"> controls in src/features/settings/components/sections/SettingsComposerSection.tsx, with matching CSS updates in src/styles/settings.css.
| type SendUserMessageToThread = ( | ||
| workspace: WorkspaceInfo, | ||
| threadId: string, | ||
| message: string, | ||
| imageIds: string[], | ||
| options?: SendUserMessageOptions, | ||
| ) => Promise<void>; | ||
| ) => Promise<unknown>; | ||
|
|
There was a problem hiding this comment.
sendUserMessageToThread was widened to Promise<unknown>, which loses type safety across the messaging stack. Consider aligning this with the concrete send result type returned by thread messaging (or a void | result union) so callers can reliably reason about outcomes without resorting to unknown.
There was a problem hiding this comment.
Implemented in 190a5fc.
Narrowed sendUserMessageToThread typing in src/features/app/hooks/usePlanReadyActions.ts to Promise<void | SendMessageResult> and reused shared SendMessageResult from src/types.ts.
| onFileAutocompleteActiveChange, | ||
| contextActions = [], | ||
| }: ComposerProps) { | ||
| void onQueue; |
There was a problem hiding this comment.
onQueue is no longer used by the component (it’s only referenced via void onQueue;). Keeping an unused, optional prop and silencing it with void makes the API confusing and easy to misuse. Consider removing onQueue from ComposerProps and its call sites (layout nodes/App wiring), or reintroduce a real queue handler if it’s still needed.
| void onQueue; |
There was a problem hiding this comment.
Implemented in 819a1c1.
Removed onQueue from ComposerProps and all call sites (App.tsx, layout nodes, orchestration, PR composer/tests) to keep the composer API explicit and remove dead plumbing.
| rateLimitsByWorkspace, | ||
| pendingInterruptsRef, | ||
| dispatch, | ||
| getCustomName, | ||
| getCustomName: _getCustomName, | ||
| markProcessing, | ||
| markReviewing, |
There was a problem hiding this comment.
getCustomName is destructured as _getCustomName and is not referenced anywhere in this hook. Keeping it as a required option increases surface area and suggests behavior that doesn’t exist. Either remove it from UseThreadMessagingOptions/call sites, or use it (e.g., when generating optimistic thread metadata) so the parameter is meaningful.
There was a problem hiding this comment.
Implemented in ee34c3a.
getCustomName is now used in useThreadMessaging debug payload (threadCustomName) so the dependency is meaningful and no longer dead.
| collaborationMode?: Record<string, unknown> | null; | ||
| }, | ||
| ) => Promise<void>; | ||
| ) => Promise<unknown>; |
There was a problem hiding this comment.
The sendUserMessageToThread callback type was widened to Promise<unknown>, which discards useful type information and reduces type safety for callers. If the intent is to accommodate the new SendMessageResult return shape, consider introducing a shared exported result type (or using Promise<void | SendMessageResult>) instead of unknown.
| ) => Promise<unknown>; | |
| ) => Promise<void>; |
There was a problem hiding this comment.
Implemented in 190a5fc.
Updated src/features/workspaces/hooks/useWorkspaceHome.ts callback typing to Promise<void | SendMessageResult> and reused shared SendMessageResult for consistency and type-safety.
| sendUserMessageToThread: ( | ||
| workspace: WorkspaceInfo, | ||
| threadId: string, | ||
| text: string, | ||
| images?: string[], | ||
| ) => Promise<void>; | ||
| ) => Promise<unknown>; | ||
| }; |
There was a problem hiding this comment.
The sendUserMessageToThread callback is now typed as returning Promise<unknown>, which makes it easy to accidentally ignore/forget send outcomes. Prefer a shared explicit return type (e.g. the new send result shape) so hooks composing this behavior can remain type-safe while still choosing to ignore the result.
There was a problem hiding this comment.
Implemented in 190a5fc.
Updated sendUserMessageToThread typing in src/features/git/hooks/usePullRequestReviewActions.ts to Promise<void | SendMessageResult> and reused shared SendMessageResult.
amanthanvi
force-pushed
the
feat/followup-queue-steer-460
branch
from
e6f006e to
415c549
Compare
|
Correction to prior note (shell escaped): branch history cleanup is complete. I force-pushed |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="composer-followup-copy"> | ||
| {oppositeFallsBackToQueue ? ( | ||
| <> | ||
| Default: Queue (Steer unavailable). Press {followUpShortcutLabel} to queue this |
There was a problem hiding this comment.
When steer is unavailable and user presses the opposite shortcut, the hint says "Press Shift+Cmd+Enter to queue this message", but that doesn't make sense since the default is already Queue and the opposite would also fall back to Queue. The hint should clarify that both actions will queue when steer is unavailable, or omit mentioning the shortcut entirely in this case.
| Default: Queue (Steer unavailable). Press {followUpShortcutLabel} to queue this | |
| Default: Queue (Steer unavailable). All follow-up options will queue this |
There was a problem hiding this comment.
Implemented in 819a1c1.
Updated the steer-unavailable hint copy in src/features/composer/components/Composer.tsx to explicitly state both actions queue: “Both Enter and Shift+Cmd/Ctrl+Enter will queue this message.”
Also added assertion coverage in src/features/composer/components/ComposerSend.test.tsx.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 415c549268
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| markProcessing(threadId, false); | ||
| setActiveTurnId(threadId, null); |
There was a problem hiding this comment.
When a turn/steer request returns an RPC error, this branch clears isProcessing and activeTurnId immediately, even though the original turn may still be running (for example, transient RPC/network failures during an active turn). That stale reset interacts badly with the queue flusher (useQueuedSend only waits for isProcessing to be false), so a queued follow-up can be sent as a new turn before the original one finishes, leading to rejected or dropped follow-ups. For steer failures, keep the existing processing/turn state and only queue the message for retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in ee34c3a.
useThreadMessaging no longer clears isProcessing / activeTurnId on steer RPC errors or thrown steer failures. It now preserves in-flight state for steer mode and returns steer_failed so queued fallback can retry safely without forcing an early flush.
Updated coverage in src/features/threads/hooks/useThreadMessaging.test.tsx to assert stale-state clearing does not happen on steer failures.
|
Addressed the current Copilot/Codex review feedback in three commits:
Validation run on this branch:
I also replied inline on each relevant review thread with file-level implementation notes. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 190a5fcadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sendResult.status === "steer_failed" && | ||
| activeThreadId && | ||
| isProcessing | ||
| ) { | ||
| enqueueMessage(activeThreadId, createQueuedItem(trimmed, nextImages, nextMentions)); |
There was a problem hiding this comment.
When sendUserMessage returns steer_failed, this block appends the message to the local queue, but useThreadMessaging.sendMessageToThread does not clear processing/turn state for steer failures (see useThreadMessaging.ts around lines 266-279), and the queue flusher bails out whenever isProcessing is true (useQueuedSend.ts lines 371-373). In the real failure case this code targets (for example, server responds no active turn to steer), that leaves follow-ups stuck in queue indefinitely unless some unrelated event later clears the stale status.
Useful? React with 👍 / 👎.
2 participants
Summary
Implements official Codex-style follow-up behavior parity for Queue vs Steer while a turn is processing.
followUpMessageBehavior(queue | steer) with Rust + TS migration from legacysteerEnabledShift+Cmd+Enter(macOS) /Shift+Ctrl+Enter(Windows/Linux)Tabqueue shortcutdefault | queue | steer) through composer/thread messagingUX Behavior
Enterand opposite shortcut both send normal messagesEnterqueues; opposite shortcut steers when availableEntersteers when available; opposite shortcut queuesFiles/Areas
src/types.ts,src/features/settings/hooks/useAppSettings.ts,src-tauri/src/types.rs,src-tauri/src/storage.rssrc/features/settings/components/sections/SettingsComposerSection.tsx,src/features/settings/hooks/useSettingsFeaturesSection.ts,src/features/settings/hooks/useSettingsViewOrchestration.ts,src/styles/settings.csssrc/features/composer/components/Composer.tsx,src/features/composer/components/ComposerInput.tsx,src/features/threads/hooks/useQueuedSend.ts,src/features/threads/hooks/useThreadMessaging.ts,src/features/app/hooks/useComposerController.ts,src/features/layout/hooks/layoutNodes/*,src/App.tsxREADME.md,docs/app-server-events.md,AGENTS.mdValidation
Commands run:
npm run typechecknpm run lintnpm run test -- src/features/settings/components/SettingsView.test.tsxnpm run test -- src/features/threads/hooks/useQueuedSend.test.tsx src/features/threads/hooks/useThreadMessaging.test.tsx src/features/composer/components/ComposerSend.test.tsxnpm run test -- src/features/settings/components/SettingsView.test.tsx src/features/git/hooks/usePullRequestComposer.test.tsx src/features/composer/components/ComposerEditorHelpers.test.tsx src/features/threads/hooks/useThreads.integration.test.tsxnpm run test -- src/features/settings/components/SettingsView.test.tsx src/features/composer/components/ComposerSend.test.tsx src/features/threads/hooks/useThreadMessaging.test.tsx src/features/threads/hooks/useQueuedSend.test.tsx src/features/git/hooks/usePullRequestComposer.test.tsxcd src-tauri && cargo test read_settings_migrates_follow_up_behaviorcd src-tauri && cargo test read_settings_keeps_existing_follow_up_behaviorKnown pre-existing lint warning (unrelated):
src/features/layout/components/DesktopLayout.tsxReact hooks dependency warningTracking Notes
PLAN.md,SPEC.md) were updated and are intentionally excluded from the PR.Issue Links
Refs #343
Closes #464
Closes #465