Fixing Cli Send Error#318
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds chat-info snapshot and RightPane UI, chat text selection and copy/keyboard/mouse support, runtime-activity aggregation and rendering, Codex spawn/permission and goal-budget handling with backoff, terminal hydration/backfill and Shift-mouse reporting, subagent identity resolution, PTY env handling, session transcript tail action, tests, and a dev CLI build-freshness helper. ChangesTUI and Desktop Client Upgrade
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
@copilot review but do not make fixes |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Reviewed commit 24dc32f only (no fixes made). I don’t see a blocking issue in the CLI send/steer fallback logic from this patch. Current PR CI run 26016037003 is still in progress with no failing jobs yet; the most recent failed dynamic run I checked failed from Copilot weekly rate limit, not code. Local targeted tests on this runner were blocked by environment mismatch (Node 20 vs required Node >=22, plus |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
ade/fixing-cli-send-error-099dff5b
branch
from
24dc32f to
781fde6
Compare
|
@copilot review but do not make fixes |
Reviewed latest commit 781fde6 without making fixes. I can confirm the three Greptile points are still valid in current code: (1) |
|
@copilot review but do not make fixes |
Reviewed latest commit cb0d921 without making fixes. Both newly flagged TerminalView points are valid in current code: (1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ade-cli/src/tuiClient/subagentPane.ts (1)
63-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMouse row offset math is out of sync with rendered roster row heights.
Line 77-79 always adds an extra line for rows with
lastToolName/summary, but chat-info only renders that detail line for the currently selected row. This can mis-map click lines to the wrong subagent index.Suggested fix
export function subagentPaneSelectableLineOffsets( content: SubagentPaneContent, + selectedIndex = 0, ): number[] { const rows = buildSubagentPaneRows(content); const offsets: number[] = []; let line = SUBAGENT_PANE_TABLE_START_LINE; for (let index = 0; index < rows.length; index += 1) { const row = rows[index]!; const previous = rows[index - 1]; const showSection = row.section !== "main" && previous?.section !== row.section; if (showSection) line += 2; offsets.push(line); line += 1; - if (row.kind === "main" || row.snapshot.lastToolName || row.snapshot.summary) { + const isSelectedSnapshot = row.kind === "snapshot" && index === selectedIndex; + if (row.kind === "main" || (isSelectedSnapshot && (row.snapshot.lastToolName || row.snapshot.summary))) { line += 1; } } return offsets; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/subagentPane.ts` around lines 63 - 80, The selectable-line offset math in subagentPaneSelectableLineOffsets is adding a detail line for any row that has snapshot.lastToolName or snapshot.summary, but the UI only renders that detail for the currently selected row; change the function signature (subagentPaneSelectableLineOffsets) to accept the currently selected row index (e.g., selectedIndex: number) and modify the loop so the extra line is only added when index === selectedIndex (i.e., when the detail is actually rendered); also update any callers of subagentPaneSelectableLineOffsets to pass the selected index so offsets stay in sync with rendered rows.apps/ade-cli/src/tuiClient/app.tsx (1)
2466-2474:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the transcript selection when switching inspected subagents.
displayEventsflips between the main chat and a subagent transcript here, but the existingchatMouseSelectionis preserved. That leaves a stale range pointing at unrelated rows in the new transcript, so highlight/copy can act on the wrong content until the user clears it manually.Suggested fix
const displayEvents = useMemo(() => ( selectedAgentSnapshot ? buildSubagentTranscriptEvents({ events, activeSession, snapshot: selectedAgentSnapshot }) : events ), [activeSession, events, selectedAgentSnapshot]); + + useEffect(() => { + chatSelectionAnchorRef.current = null; + updateChatMouseSelection(null); + }, [selectedAgentSnapshot, updateChatMouseSelection]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 2466 - 2474, When switching inspected subagents the existing chatMouseSelection can point to rows that no longer match displayEvents; add logic to clear the selection whenever the inspected subagent or the selectedAgentSnapshot context changes. Concretely, add a useEffect that depends on inspectedSubagentId (or selectedAgentSnapshot, rightOpen/rightPane as needed) and calls the chatMouseSelection reset (e.g. setChatMouseSelection(null) or appropriate clear function) so selection is cleared whenever displayEvents switches between main chat and a subagent transcript.
🧹 Nitpick comments (3)
apps/ade-cli/src/adeRpcServer.test.ts (1)
2406-2406: ⚡ Quick winConsider cleaning up the temporary directory after the test.
The temporary directory created by
fs.mkdtempSyncis not removed after the test completes, which could accumulate over multiple test runs. Some tests in this file usetry-finallywithfs.rmSyncto clean up temporary resources (see lines 1872-1888).♻️ Suggested cleanup pattern
it("launches default Codex spawn_agent sessions with supported sandbox flags", async () => { const fixture = createRuntime(); const binDir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-cli-spawn-codex-bin-")); - createFakePathExecutable(binDir, "codex"); - const handler = createAdeRpcRequestHandler({ runtime: fixture.runtime, serverVersion: "test" }); - - const response = await withEnv({ PATH: `${binDir}${path.delimiter}${process.env.PATH ?? ""}`, SHELL: "/bin/sh" }, async () => { - await initialize(handler, { role: "orchestrator" }); - return await callTool(handler, "spawn_agent", { - laneId: "lane-1", - provider: "codex", - permissionMode: "default", - prompt: "Check the Codex launch flags", - }); - }); - - expect(response?.isError).toBeUndefined(); - const createCall = fixture.runtime.ptyService.create.mock.calls[0]?.[0] as { args?: string[]; startupCommand?: string }; - expect(createCall.args).toEqual(expect.arrayContaining(["--sandbox", "workspace-write", "--ask-for-approval", "on-request"])); - expect(createCall.args).not.toContain("--full-auto"); - expect(createCall.startupCommand).toContain("--sandbox workspace-write --ask-for-approval on-request"); - expect(createCall.startupCommand).not.toContain("--full-auto"); + try { + createFakePathExecutable(binDir, "codex"); + const handler = createAdeRpcRequestHandler({ runtime: fixture.runtime, serverVersion: "test" }); + + const response = await withEnv({ PATH: `${binDir}${path.delimiter}${process.env.PATH ?? ""}`, SHELL: "/bin/sh" }, async () => { + await initialize(handler, { role: "orchestrator" }); + return await callTool(handler, "spawn_agent", { + laneId: "lane-1", + provider: "codex", + permissionMode: "default", + prompt: "Check the Codex launch flags", + }); + }); + + expect(response?.isError).toBeUndefined(); + const createCall = fixture.runtime.ptyService.create.mock.calls[0]?.[0] as { args?: string[]; startupCommand?: string }; + expect(createCall.args).toEqual(expect.arrayContaining(["--sandbox", "workspace-write", "--ask-for-approval", "on-request"])); + expect(createCall.args).not.toContain("--full-auto"); + expect(createCall.startupCommand).toContain("--sandbox workspace-write --ask-for-approval on-request"); + expect(createCall.startupCommand).not.toContain("--full-auto"); + } finally { + fs.rmSync(binDir, { recursive: true, force: true }); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/adeRpcServer.test.ts` at line 2406, The test creates a temp directory in the variable binDir via fs.mkdtempSync("ade-cli-spawn-codex-bin-") but never removes it; update the test to wrap the setup and usage of binDir in a try/finally (mirroring existing patterns around lines 1872-1888) and call fs.rmSync(binDir, { recursive: true, force: true }) in the finally block so the temporary directory is always cleaned up after the test.apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
233-233: ⚡ Quick winRename constant to camelCase to match repository naming rules.
TUI_FRAME_CHARSshould be camelCase in this TSX file.♻️ Proposed fix
-const TUI_FRAME_CHARS = /[╭╮╰╯│─┌┐└┘├┤┬┴┼▐▌▀▄█▛▜▝▘]/; +const tuiFrameChars = /[╭╮╰╯│─┌┐└┘├┤┬┴┼▐▌▀▄█▛▜▝▘]/; @@ - if (TUI_FRAME_CHARS.test(text)) return true; + if (tuiFrameChars.test(text)) return true;As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.Also applies to: 257-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx` at line 233, The constant TUI_FRAME_CHARS in WorkViewArea.tsx violates the camelCase naming rule; rename the identifier to tuiFrameChars across the file (including its definition and every usage, e.g., the other occurrence around line 257) while keeping its regex value unchanged and preserving any exports or imports that reference it so callers are updated to the new name.apps/desktop/src/shared/cliLaunch.ts (1)
61-64: ⚡ Quick winRename the new constant to camelCase for guideline compliance.
Line 61 introduces
ADE_CODEX_STARTUP_CONFIG_FLAGS, which breaks the repo naming rule for TS variables/constants. Please rename it to camelCase and update usages.Suggested diff
-const ADE_CODEX_STARTUP_CONFIG_FLAGS = [ +const adeCodexStartupConfigFlags = [ "-c", "mcp_servers.linear.enabled=false", ] as const; @@ - const codexStartupConfigFlags = args.permissionMode === "config-toml" ? [] : ADE_CODEX_STARTUP_CONFIG_FLAGS; + const codexStartupConfigFlags = args.permissionMode === "config-toml" ? [] : adeCodexStartupConfigFlags;As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.Also applies to: 206-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/cliLaunch.ts` around lines 61 - 64, Rename the exported constant ADE_CODEX_STARTUP_CONFIG_FLAGS to camelCase (e.g., adeCodexStartupConfigFlags) in the declaration and update every usage site to the new name; modify the const declaration in cliLaunch.ts (the array currently defined as ADE_CODEX_STARTUP_CONFIG_FLAGS) and replace references elsewhere in this file (also around the other related block at lines ~206-213) so imports/usages match the new identifier, keeping the value and const/readonly typing intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 4153-4160: In copyChatSelection, don't call .trim() on the string
returned by selectedTextFromChatRows (used with selectableChatRowTextsRef and
chatMouseSelectionRef) because that mutates the user's selection (breaking code
blocks and exact transcripts); remove the .trim() and change the subsequent
emptiness check to test the raw string (e.g., text.length === 0 or !text) so
leading/trailing whitespace and newlines are preserved when copying.
- Around line 1597-1600: The auto-scroll triggers while the pointer is still on
the first/last visible row because the checks use inclusive comparisons; change
the edge checks in the function that computes scroll direction (using variables
y, topRow, bottomRow, scrollOffsetRows, maxScrollOffsetRows, rowBudget) from
inclusive to strict comparisons so scrolling starts only after the pointer
leaves the visible transcript—replace "y <= topRow" with "y < topRow" and "y >=
bottomRow" with "y > bottomRow".
In `@apps/ade-cli/src/tuiClient/components/ChatView.tsx`:
- Around line 903-907: The helper activeTurnRows currently treats any
"assistant-text" anywhere in blocks as meaning the active turn has assistant
output; change it to only look for assistant outputs in the active (tail) turn
instead of the whole transcript: locate the start of the current/active turn
(e.g., by finding the last user/turn boundary in blocks) and then run the
hasAssistantOutput check only over the blocks after that boundary; update
activeTurnRows accordingly (and any other consumer like
computeChatScrollMaxOffset that reuses this helper) so the waiting/streaming row
is rendered for new turns even after earlier assistant replies exist.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 2435-2449: The code currently sets explicitNoColor using
hasEnvValue which treats an empty NO_COLOR ("") as unset; change the
explicitNoColor check to detect the presence of the NO_COLOR key (not just a
non-empty value). Update the expression that sets explicitNoColor (currently
calling hasEnvValue on args.env and laneRuntimeEnv) to use a key-presence check
(e.g., hasEnvKey or Object.prototype.hasOwnProperty.call(...) on args.env and
laneRuntimeEnv) so that NO_COLOR="" is considered true before calling
withInteractiveTerminalColorEnv; keep the rest of the launchEnv flow
(baseLaunchEnv, withAdeTerminalContextEnv, getAdeCliAgentEnv,
withInteractiveTerminalColorEnv) unchanged.
---
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 2466-2474: When switching inspected subagents the existing
chatMouseSelection can point to rows that no longer match displayEvents; add
logic to clear the selection whenever the inspected subagent or the
selectedAgentSnapshot context changes. Concretely, add a useEffect that depends
on inspectedSubagentId (or selectedAgentSnapshot, rightOpen/rightPane as needed)
and calls the chatMouseSelection reset (e.g. setChatMouseSelection(null) or
appropriate clear function) so selection is cleared whenever displayEvents
switches between main chat and a subagent transcript.
In `@apps/ade-cli/src/tuiClient/subagentPane.ts`:
- Around line 63-80: The selectable-line offset math in
subagentPaneSelectableLineOffsets is adding a detail line for any row that has
snapshot.lastToolName or snapshot.summary, but the UI only renders that detail
for the currently selected row; change the function signature
(subagentPaneSelectableLineOffsets) to accept the currently selected row index
(e.g., selectedIndex: number) and modify the loop so the extra line is only
added when index === selectedIndex (i.e., when the detail is actually rendered);
also update any callers of subagentPaneSelectableLineOffsets to pass the
selected index so offsets stay in sync with rendered rows.
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Line 2406: The test creates a temp directory in the variable binDir via
fs.mkdtempSync("ade-cli-spawn-codex-bin-") but never removes it; update the test
to wrap the setup and usage of binDir in a try/finally (mirroring existing
patterns around lines 1872-1888) and call fs.rmSync(binDir, { recursive: true,
force: true }) in the finally block so the temporary directory is always cleaned
up after the test.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Line 233: The constant TUI_FRAME_CHARS in WorkViewArea.tsx violates the
camelCase naming rule; rename the identifier to tuiFrameChars across the file
(including its definition and every usage, e.g., the other occurrence around
line 257) while keeping its regex value unchanged and preserving any exports or
imports that reference it so callers are updated to the new name.
In `@apps/desktop/src/shared/cliLaunch.ts`:
- Around line 61-64: Rename the exported constant ADE_CODEX_STARTUP_CONFIG_FLAGS
to camelCase (e.g., adeCodexStartupConfigFlags) in the declaration and update
every usage site to the new name; modify the const declaration in cliLaunch.ts
(the array currently defined as ADE_CODEX_STARTUP_CONFIG_FLAGS) and replace
references elsewhere in this file (also around the other related block at lines
~206-213) so imports/usages match the new identifier, keeping the value and
const/readonly typing intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 046c8275-f13b-4ba1-8299-63df26bc1bc6
⛔ Files ignored due to path filters (5)
docs/features/ade-code/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**goal.mdis excluded by!*.md
📒 Files selected for processing (41)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/HeaderFooter.test.tsxapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/aggregate.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/chatInfo.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/__tests__/keybindings.test.tsapps/ade-cli/src/tuiClient/__tests__/subagentPane.test.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/chatInfo.tsapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/FooterControls.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/subagentPane.tsapps/ade-cli/src/tuiClient/types.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/chat/codex/CodexGoalBanner.test.tsxapps/desktop/src/renderer/components/chat/codex/CodexGoalBanner.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/cliLaunch.tsscripts/dev-shared.mjs
|
@copilot review but do not make fixes |
Reviewed latest commit d1eafbf without making fixes. Current status of the three flagged points: (1) |
|
@copilot review but do not make fixes |
Reviewed latest commit 2800dd5 without making fixes. Both currently flagged points remain valid in code: (1) |
|
@copilot review but do not make fixes |
Reviewed latest commit aa10498 without making fixes. The newly flagged Ctrl+C ordering issue is still valid on Windows: in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
7902-7966: 💤 Low valueConsider isolating post-success work from the API error handler.
The
.catch()handler will also catch exceptions thrown by code in.then()(e.g., ifpersistChatStatewere to throw). This would incorrectly set a retry timestamp and emit an error notice even though thethread/goal/setrequest actually succeeded.If
persistChatStateis guaranteed not to throw, this is fine. Otherwise, consider wrapping the.then()body in a try-catch or restructuring to avoid conflating API failures with post-processing failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 7902 - 7966, The API error handler conflates network/request failures with exceptions thrown during post-success processing (the .then() callback), so change maybeClearCodexGoalBudget to isolate post-success work: inside the .then(response => { ... }) wrap the block that updates managed.session.codexGoal, calls emitChatEvent, and calls persistChatState(managed) in its own try-catch; on post-success catch emit a system_notice error (referencing persistChatState, emitChatEvent, and updatedGoal) but do NOT rethrow so runtime.request rejection handling (where runtime.goalBudgetClearRetryAfterByThreadId is set) only runs for actual request errors from runtime.request("thread/goal/set"); leave the .catch() to handle real API failures and keep the .finally() that deletes runtime.goalBudgetClearInFlight.apps/desktop/src/renderer/components/terminals/TerminalView.tsx (1)
99-101: ⚡ Quick winUse camelCase for newly introduced constants.
HYDRATION_BACKFILL_RETRY_MS,HYDRATION_VISIBLE_BLANK_BACKFILL_RETRY_MS,HYDRATION_BACKFILL_MAX_ATTEMPTS, andTERMINAL_MOUSE_TRACKING_EVENT_MODESviolate the repository naming rule for TS/JS variables.As per coding guidelines
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx` around lines 99 - 101, Rename the newly introduced constants to camelCase (e.g., HYDRATION_BACKFILL_RETRY_MS -> hydrationBackfillRetryMs, HYDRATION_VISIBLE_BLANK_BACKFILL_RETRY_MS -> hydrationVisibleBlankBackfillRetryMs, HYDRATION_BACKFILL_MAX_ATTEMPTS -> hydrationBackfillMaxAttempts, and TERMINAL_MOUSE_TRACKING_EVENT_MODES -> terminalMouseTrackingEventModes), update all references/usages in TerminalView.tsx (and any imports/exports or tests) to the new identifiers, and ensure any exported names or destructured usages are adjusted accordingly so the code compiles with the repository's camelCase naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 1011-1024: The backfill attempt counter
(runtime.hydrationBackfillAttempts) is being incremented when scheduling a
timer, allowing rapid timer churn (via options.replaceExistingTimer) to consume
the retry budget; instead, increment the counter only when an actual backfill
attempt will run. Move the runtime.hydrationBackfillAttempts += 1 out of the
scheduling path into the setTimeout callback (or immediately before calling
readPreviewHydrationData) so canceled/replaced timers do not count, and ensure
the logic around hydrationBackfillTimer clearing (and
options.replaceExistingTimer) does not increment attempts prematurely; reference
the needsHydrationBackfill check, runtime.hydrationBackfillTimer,
hydrationBackfillAttempts, the setTimeout callback, and readPreviewHydrationData
to locate and update the code.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 7902-7966: The API error handler conflates network/request
failures with exceptions thrown during post-success processing (the .then()
callback), so change maybeClearCodexGoalBudget to isolate post-success work:
inside the .then(response => { ... }) wrap the block that updates
managed.session.codexGoal, calls emitChatEvent, and calls
persistChatState(managed) in its own try-catch; on post-success catch emit a
system_notice error (referencing persistChatState, emitChatEvent, and
updatedGoal) but do NOT rethrow so runtime.request rejection handling (where
runtime.goalBudgetClearRetryAfterByThreadId is set) only runs for actual request
errors from runtime.request("thread/goal/set"); leave the .catch() to handle
real API failures and keep the .finally() that deletes
runtime.goalBudgetClearInFlight.
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 99-101: Rename the newly introduced constants to camelCase (e.g.,
HYDRATION_BACKFILL_RETRY_MS -> hydrationBackfillRetryMs,
HYDRATION_VISIBLE_BLANK_BACKFILL_RETRY_MS ->
hydrationVisibleBlankBackfillRetryMs, HYDRATION_BACKFILL_MAX_ATTEMPTS ->
hydrationBackfillMaxAttempts, and TERMINAL_MOUSE_TRACKING_EVENT_MODES ->
terminalMouseTrackingEventModes), update all references/usages in
TerminalView.tsx (and any imports/exports or tests) to the new identifiers, and
ensure any exported names or destructured usages are adjusted accordingly so the
code compiles with the repository's camelCase naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b408f15-529e-4989-aa9a-45334e90d81b
📒 Files selected for processing (16)
apps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/subagentPane.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/subagentPane.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/main/utils/terminalSessionSignals.test.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/ade-cli/src/tuiClient/tests/subagentPane.test.ts
- apps/ade-cli/src/tuiClient/tests/ChatView.test.tsx
- apps/ade-cli/src/tuiClient/tests/appInput.test.ts
- apps/ade-cli/src/tuiClient/components/ChatView.tsx
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/ade-cli/src/tuiClient/app.tsx
|
@copilot review but do not make fixes |
Reviewed latest commit 425438d without making fixes. The newly flagged TerminalView backfill-budget issue appears addressed: |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Greptile Summary
This PR introduces several improvements to the ADE CLI and desktop app: a new Chat Info panel (replacing the Subagents pane), chat text-selection with clipboard copy, a change to Codex default permissions (from
--full-autoto--sandbox workspace-write --ask-for-approval on-request), and terminal hydration/backfill retry logic.subagentspane with a unifiedchat-infopane showing provider/model, plan, goal, and a subagent roster. The default right-pane content now shows for any active session (not just sessions with live agents), and a newderiveChatInfoSnapshotfunction drives the data.cliLaunch.ts,adeRpcServer.ts, andterminalSessionSignals.ts. ThesanitizeTrackedCliResumeTargetIdregex is also tightened with an explicit character whitelist.NO_COLOR/FORCE_COLOR) is now preserved correctly; hydration backfill attempts counter is incremented inside the timer callback; Codex goal budget clear retries use a 30-second backoff; stopped sub-agents now emitnoticeKind: "info"instead of"error".Confidence Score: 4/5
Safe to merge; the most impactful changes (permission hardening, color env fix, hydration retry, goal budget backoff) all look correct.
All five previously flagged concerns are addressed in this PR. The remaining findings are minor style issues in the new Chat Info panel that do not affect runtime correctness.
apps/ade-cli/src/tuiClient/components/RightPane.tsx — two small polish items in the new ChatInfoPlanBlock.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant App as AdeCodeApp (app.tsx) participant ChatView as ChatView.tsx participant CI as chatInfo.ts participant RightPane as RightPane.tsx participant ACS as agentChatService.ts participant PTY as ptyService.ts User->>App: Open session / /info command App->>CI: deriveChatInfoSnapshot(events, session, ...) CI-->>App: ChatInfoSnapshot App->>RightPane: "setRightPane({ kind: chat-info, info })" RightPane-->>User: ChatInfoHeader + Plan + Goal + Roster User->>App: Shift+drag / Meta+C App->>ChatView: renderChatVisibleSelectionRows(...) ChatView-->>App: ChatVisibleSelectionRow[] (with sourceRowIndex) App->>ChatView: renderChatSelectableRowTexts(...) ChatView-->>App: string[] (selectable row texts) App->>App: selectedTextFromChatRows(rows, selection) App-->>User: clipboard write ACS->>ACS: thread/goal/updated notification ACS->>ACS: applyCodexGoalUpdate ACS->>ACS: maybeClearCodexGoalBudget (30s backoff) ACS-->>App: codex_goal_updated event PTY->>PTY: withInteractiveTerminalColorEnv(env, preserveNoColor) PTY->>PTY: hasEnvKey check for NO_COLOR PTY-->>App: PTY spawned with correct color envComments Outside Diff (1)
apps/desktop/src/main/services/chat/agentChatService.ts, line 12472-12480 (link)When
spawnStatus === "stopped"(which includescancelled,canceled,interrupted,shutdown, andnotfound), the code still emits asystem_noticewithnoticeKind: "error"andseverity: "error". For intentional cancellations — for instance, when the user stops the main orchestration session and all sub-agents receive a "cancelled" status — every sub-agent will surface a red error banner. The fallback message inreadCodexCollabFailureSummary("Codex rejected the parallel agent request.") compounds this by framing a normal stop as a rejection. Consider usingnoticeKind: "info"(and droppingseverity: "error") for the "stopped" path so that the UI only shows a hard error whenspawnStatus === "failed".Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "ship: iteration 5 - fix terminal backfil..." | Re-trigger Greptile