[codex] Add desktop plan and goal mode UI - #55651
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary - COMMENT - High Surface Area. 18 files changed with 907 additions. This PR adds desktop plan and goal mode UI for Codex. High file count (18 files) exceeds the 15-file surface-area threshold. This is a significant UI feature addition. Would benefit from a focused review of the UI components and state management. Consider splitting into smaller PRs if possible, or noting the specific files that are critical vs. mechanical. Reviewed by Hermes Agent
|
Thank you for your careful review. If you still think splitting is better for review, I am willing to reorganize the code and submit smaller incremental PRs step by step later. |
|
Gentle reminder, could you approve the waiting workflow to trigger the required checks? Thank you. |
|
Thanks for the review note. To make the surface area easier to inspect, here is the structure of this PR:
The 18 touched files are connected because the desktop composer controls, session status stack, and existing todo/goal gateway paths need to work together for the feature to be usable end-to-end. If maintainers prefer, I can split this into smaller incremental PRs:
Could you please approve the waiting workflow so the required checks can run first? |
19df415 to
f8e757a
Compare
|
Resolved the latest merge conflict in The conflict was only around the React import line after Validation run locally:
GitHub now reports the PR as mergeable; it appears to be waiting for the required workflow/check approval to proceed. |
|
Since this PR has stayed blocked on workflow approval for a while, I started splitting it into smaller, independently reviewable PRs. First split PR: #63527 — desktop Goal status plumbing/display only. That PR reduces the surface area to 9 files and intentionally excludes the composer Goal button and Plan mode flow. It is locally validated with typecheck, the focused goal store test, and targeted ESLint. I am keeping this larger PR open for context while maintainers decide which route is easier to review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for splitting the Desktop goal/plan work around the existing gateway and status-stack seams.
Problems
apps/desktop/src/app/chat/composer/index.tsx:750callsclearSessionTodos(statusSessionId)on plan cancellation.apps/desktop/src/store/todos.ts:64-74removes the entire todo entry for that session, so cancelling a plan also erases unrelated todo state.- The new copy changes only
apps/desktop/src/i18n/en.tsandapps/desktop/src/i18n/zh.ts; Desktop also shipsjaandzh-hantthroughapps/desktop/src/i18n/catalog.ts:1-11. - The added tests are store-only (
apps/desktop/src/store/goals.test.ts,apps/desktop/src/store/plans.test.ts), leaving the new composer submit/approve/cancel path untested.
Suggested changes
- Make cancellation remove only plan-owned state; it must not clear the session's ordinary todos.
- Add Japanese and Traditional Chinese copy.
- Add composer-level coverage for plan submission, approval, rejection recovery, and cancellation preserving existing todos.
Automated hermes-sweeper review.
| clearPendingPlan(planSessionKey) | ||
|
|
||
| if (statusSessionId) { | ||
| clearSessionTodos(statusSessionId) |
There was a problem hiding this comment.
clearSessionTodos() deletes the entire runtime-session todo list (src/store/todos.ts:64-74), including work unrelated to this pending plan. Please remove only plan-owned state here, or preserve and restore existing todos.
…locale copy Follow-ups on the salvaged goal-status display (#63527): - Seed the goal store from the /goal dispatch notice ("⊙ Goal set …") and from /goal status|pause|resume|clear exec output in slash.ts. The backend only emits status.update kind:"goal" after the first turn's post-turn judge, so without this the indicator stayed empty while the kickoff turn ran (sweeper review finding on #63527). - Add the missing ja / zh-hant statusStack goal copy — desktop ships four locales, not two (sweeper review finding on #55651). - Add a component-level vitest for the composer goal indicator rendering from store states: none / active / paused / detail line / other-session. Co-authored-by: HaisamAbbas <95044189+HaisamAbbas@users.noreply.github.com> Assisted-by: Claude Fable 5 via Hermes Agent
…locale copy Follow-ups on the salvaged goal-status display (#63527): - Seed the goal store from the /goal dispatch notice ("⊙ Goal set …") and from /goal status|pause|resume|clear exec output in slash.ts. The backend only emits status.update kind:"goal" after the first turn's post-turn judge, so without this the indicator stayed empty while the kickoff turn ran (sweeper review finding on #63527). - Add the missing ja / zh-hant statusStack goal copy — desktop ships four locales, not two (sweeper review finding on #55651). - Add a component-level vitest for the composer goal indicator rendering from store states: none / active / paused / detail line / other-session. Co-authored-by: HaisamAbbas <95044189+HaisamAbbas@users.noreply.github.com> Assisted-by: Claude Fable 5 via Hermes Agent
…locale copy Follow-ups on the salvaged goal-status display (NousResearch#63527): - Seed the goal store from the /goal dispatch notice ("⊙ Goal set …") and from /goal status|pause|resume|clear exec output in slash.ts. The backend only emits status.update kind:"goal" after the first turn's post-turn judge, so without this the indicator stayed empty while the kickoff turn ran (sweeper review finding on NousResearch#63527). - Add the missing ja / zh-hant statusStack goal copy — desktop ships four locales, not two (sweeper review finding on NousResearch#55651). - Add a component-level vitest for the composer goal indicator rendering from store states: none / active / paused / detail line / other-session. Co-authored-by: HaisamAbbas <95044189+HaisamAbbas@users.noreply.github.com> Assisted-by: Claude Fable 5 via Hermes Agent
Summary
/goalgateway command.status.updateevents withkind: "goal"in the composer status stack.Details
This reuses the existing todo/status stack infrastructure and gateway slash command path instead of introducing a new backend API. Plan mode stores a pending plan locally, shows the todo plan in the existing composer status stack, and sends a follow-up implementation prompt only when the user confirms. Goal mode parses slash command output and goal status events into a session-scoped status row with pause/resume/clear actions.
Validation
npm run typechecknpm run test:ui -- src/store/goals.test.ts src/store/plans.test.tsnpx eslint src/app/chat/composer/controls.tsx src/app/chat/composer/hooks/use-composer-submit.ts src/app/chat/composer/index.tsx src/app/chat/composer/status-stack/index.tsx src/app/chat/composer/status-stack/status-row.tsx src/app/chat/composer/types.ts src/app/session/hooks/use-message-stream/gateway-event.ts src/app/session/hooks/use-prompt-actions/slash.ts src/app/session/hooks/use-prompt-actions/submit.ts src/app/session/hooks/use-prompt-actions/utils.ts src/i18n/en.ts src/i18n/types.ts src/i18n/zh.ts src/store/composer-status.ts src/store/goals.ts src/store/goals.test.ts src/store/plans.ts src/store/plans.test.ts