fix: reliably exit plan mode on Implement this plan#463
fix: reliably exit plan mode on Implement this plan#463amanthanvi wants to merge 3 commits intoDimillian:mainfrom
Conversation
(cherry picked from commit 847a31de6f5a3d4c59da9697c6ed445a8ca66310)
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where clicking "Implement this plan" could fail to exit plan mode, causing the next turn to either stall or continue with plan-mode behavior instead of switching to implementation. The root cause was a race condition in mode selection and missing synchronization between UI state and persisted thread parameters.
Changes:
- Added deterministic implementation mode resolution that prefers
default→code→ first non-plan mode, with explicitnullfallback - Synchronized collaboration mode state by calling both
setSelectedCollaborationModeIdandpersistThreadCodexParamstogether during plan follow-up actions - Changed from conditional to unconditional explicit collaboration mode overrides in message options to prevent stale mode inheritance
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/features/app/hooks/usePlanReadyActions.ts |
Core fix: added findImplementationMode() resolver, synchronized mode state updates, and ensured explicit turn overrides |
src/features/app/hooks/usePlanReadyActions.test.tsx |
Comprehensive regression tests covering default/code/fallback paths and plan-only edge case |
src/App.tsx |
Wired persistThreadCodexParams into usePlanReadyActions hook |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { collaborationMode: null }, | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Consider adding a test case for when the collaborationModes array is completely empty. While the current test covers the "only plan mode" scenario, an empty array is another edge case that should behave identically (returning null for implementationMode). This would provide more comprehensive coverage of the fallback logic in findImplementationMode.
| it("forces neutral mode override when no collaboration modes are available", async () => { | |
| const { | |
| result, | |
| setSelectedCollaborationModeId, | |
| persistThreadCodexParams, | |
| sendUserMessageToThread, | |
| } = renderPlanReadyActions({ | |
| collaborationModes: [], | |
| }); | |
| await act(async () => { | |
| await result.current.handlePlanAccept(); | |
| }); | |
| expect(setSelectedCollaborationModeId).toHaveBeenCalledWith(null); | |
| expect(persistThreadCodexParams).toHaveBeenCalledWith({ | |
| collaborationModeId: null, | |
| }); | |
| expect(sendUserMessageToThread).toHaveBeenCalledWith( | |
| connectedWorkspace, | |
| "thread-1", | |
| "[[cm_plan_ready:accept]] Implement this plan.", | |
| [], | |
| { collaborationMode: null }, | |
| ); | |
| }); |
There was a problem hiding this comment.
Implemented in 9d5c8e7. Added explicit coverage for an empty collaboration mode list in handlePlanAccept, asserting neutral override (collaborationMode: null) and persisted collaborationModeId: null.
| }, | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Consider adding a test case for when plan mode is not available when calling handlePlanSubmitChanges. While the code handles this gracefully by setting the mode to null (line 158), having explicit test coverage would document this edge case behavior and ensure it continues to work as expected.
| }); | |
| }); | |
| it("handles plan-change follow-up when plan mode is not available", async () => { | |
| const { | |
| result, | |
| setSelectedCollaborationModeId, | |
| persistThreadCodexParams, | |
| sendUserMessageToThread, | |
| } = renderPlanReadyActions({ | |
| collaborationModes: [makeMode("default")], | |
| }); | |
| await act(async () => { | |
| await result.current.handlePlanSubmitChanges(" Add tests "); | |
| }); | |
| expect(setSelectedCollaborationModeId).not.toHaveBeenCalled(); | |
| expect(persistThreadCodexParams).not.toHaveBeenCalled(); | |
| expect(sendUserMessageToThread).toHaveBeenCalledWith( | |
| connectedWorkspace, | |
| "thread-1", | |
| "[[cm_plan_ready:changes]] Update the plan with these changes:\n\nAdd tests", | |
| [], | |
| { | |
| collaborationMode: null, | |
| }, | |
| ); | |
| }); |
There was a problem hiding this comment.
Implemented in 9d5c8e7 with a safety adjustment: when handlePlanSubmitChanges cannot find plan mode, we now keep selected/persisted mode untouched and still send explicit neutral override (collaborationMode: null). Added regression test coverage for this path.
![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: c1f11868cd
ℹ️ 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".
| const planModeId = planMode?.id ?? null; | ||
| setSelectedCollaborationModeId(planModeId); | ||
| persistThreadCodexParams({ | ||
| collaborationModeId: planModeId, |
There was a problem hiding this comment.
When handlePlanSubmitChanges runs while collaboration modes are transiently unavailable (for example, after a disconnect/reconnect cycle), findCollaborationMode("plan") returns null, and this block immediately writes null into both UI selection and persisted thread params. Since useCollaborationModes clears modes whenever !isConnected (src/features/collaboration/hooks/useCollaborationModes.ts), this path can fire in normal reconnect flows and causes the "update plan" follow-up to be sent without plan mode while also erasing the thread’s prior plan selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in 9d5c8e7. handlePlanSubmitChanges now updates UI + persisted mode only when plan mode exists. When modes are transiently empty, it no longer clears stored selection and sends follow-up with explicit neutral override (collaborationMode: null). Added regression test for this edge case.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5c8e7819
ℹ️ 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".
| // @vitest-environment jsdom | ||
| import { act, renderHook } from "@testing-library/react"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import type { CollaborationModeOption, WorkspaceInfo } from "../../../types"; |
There was a problem hiding this comment.
Replace this relative import with a project alias to comply with the repo contract: AGENTS.md (repo root) explicitly requires frontend code to use aliases like @/* and @app/*. Introducing new relative paths here adds avoidable path fragility and violates the documented import standard that applies to this scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in f8c189b. Updated the new test module to use frontend aliases per repo contract: @/types and @app/hooks/usePlanReadyActions. Validation run: npm run test -- src/features/app/hooks/usePlanReadyActions.test.tsx and npm run typecheck both passed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c189b7af
ℹ️ 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".
| const implementationMode = findImplementationMode(); | ||
| const implementationModeId = implementationMode?.id ?? null; | ||
| setSelectedCollaborationModeId(implementationModeId); | ||
| persistThreadCodexParams({ |
There was a problem hiding this comment.
Calling persistThreadCodexParams here can write the mode to the wrong thread when the workspace is disconnected: handlePlanAccept awaits connectWorkspace, and during that await the user can switch threads, after which persistThreadCodexParams resolves the target from activeThreadIdRef.current (useThreadCodexOrchestration.ts), while sendUserMessageToThread still uses the originally captured activeThreadId. That mismatch causes thread-scoped collaborationModeId to be saved on a different thread than the one receiving the plan-followup message; the same pattern appears in handlePlanSubmitChanges.
Useful? React with 👍 / 👎.
2 participants
Summary
Implement this plannever falls back toplanmodeProblem
After clicking
Implement this plan, some turns could start with stale plan-mode behavior (or appear to stall after the initial goal block), and UI mode state could remain onplanfor the thread.Root cause
Plan-accept mode selection could race with collaboration mode availability and fallback to an unsafe selection path, while thread-scoped mode persistence was not updated during plan follow-up actions.
What changed
src/features/app/hooks/usePlanReadyActions.tsdefault->code-> first non-plan-> nonenull)collaborationModeIdto same valuecollaborationModepayload ornull)null) alongside selectioncollaborationModepayload ornull)src/App.tsxpersistThreadCodexParamsintousePlanReadyActionssrc/features/app/hooks/usePlanReadyActions.test.tsxLinked issue
Validation
Executed before branch transfer (same code/commit content):
npm run lintnpm run testnpm run typecheckcd src-tauri && cargo checknpm run test -- src/features/app/hooks/usePlanReadyActions.test.tsxnpm run test -- src/features/messages/components/Messages.test.tsx src/features/collaboration/hooks/useCollaborationModes.test.tsxNote: this branch was moved into an isolated worktree to avoid interfering with concurrent work on
feat/followup-queue-steer-460.