Tui Perf And Fucntilnaity Pass#325
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR establishes a side-channel JSON-RPC bridge from the CLI runtime daemon to the Electron desktop app for browser control, extends the TUI with new forms and commands for lane management and reasoning effort selection, refines the chat composer with attachment keyboard navigation and session configuration persistence, and propagates Claude task metadata through subagent events while adding PTY output replay to the web terminal. ChangesDesktop Bridge Infrastructure
TUI Application Features and Layout
Chat Composer, Session Config, and Subagent Metadata
Web Terminal PTY Output and Messaging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
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. |
arul28
force-pushed
the
ade/tui-perf-and-fucntilnaity-pass-241a6c0b
branch
from
20a666f to
5ae563a
Compare
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Review-only pass completed on commit |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/shared/chatSubagents.ts (1)
235-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
taskTypeagainst allowed literal values before assigning toSubagentSnapshot.
incomingTaskTypeaccepts any string without checking against the allowed literal values ("subagent" | "background" | "local_workflow" | "cron" | "other"), allowing invalid strings to be force-cast into the snapshot state.Suggested fix
+const SUBAGENT_TASK_TYPES = new Set<NonNullable<SubagentSnapshot["taskType"]>>([ + "subagent", + "background", + "local_workflow", + "cron", + "other", +]); @@ -const incomingTaskType = typeof event.taskType === "string" ? event.taskType : undefined; +const incomingTaskType = ( + typeof event.taskType === "string" && SUBAGENT_TASK_TYPES.has(event.taskType as NonNullable<SubagentSnapshot["taskType"]>) + ? event.taskType + : undefined +) as SubagentSnapshot["taskType"] | undefined; @@ ...(incomingTaskType || existing?.taskType - ? { taskType: (incomingTaskType ?? existing?.taskType) as SubagentSnapshot["taskType"] } + ? { taskType: incomingTaskType ?? existing?.taskType } : {}),🤖 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/chatSubagents.ts` around lines 235 - 250, incomingTaskType is currently any string and gets force-cast into SubagentSnapshot.taskType; validate it against the allowed literals ("subagent" | "background" | "local_workflow" | "cron" | "other") before assignment (e.g., implement an isValidTaskType(value) type guard or whitelist array) and only include taskType in the base object if incomingTaskType passes that check (or fall back to existing?.taskType if that is already valid); update the logic around incomingTaskType, the base object construction, and any places using taskType to rely on the validated value so invalid strings are never force-cast into SubagentSnapshot.apps/ade-cli/src/tuiClient/app.tsx (2)
6265-6274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackground Claude launches are dropping the selected reasoning effort.
startClaudeTerminalForPrompt()includesreasoningEffort, but this background-launch path does not. New Claude chats started with background launch will silently ignore the current effort setting and diverge from the foreground path.🛠️ Proposed fix
await startClaudeTerminalSession({ connection: conn, laneId, title: pendingNewChatTitleRef.current ?? "Claude Code", model: normalized.modelId ?? normalized.model, + reasoningEffort: normalized.reasoningEffort, permissionMode: normalized.permissionMode, initialInput: terminalPrompt.trim() ? terminalPrompt : null, cols, rows: terminalRows, });🤖 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 6265 - 6274, The background launch call to startClaudeTerminalSession is missing the reasoningEffort param and thus drops the user's selected effort; update the call site in app.tsx (the startClaudeTerminalSession invocation) to pass reasoningEffort: normalized.reasoningEffort (or the appropriate field on normalized that holds the current effort) — matching the foreground path used by startClaudeTerminalForPrompt — so the background-launched Claude session receives the same reasoningEffort value (use a fallback/default if needed).
7676-7731:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep non-text lane-delete controls out of the prompt buffer.
formFieldUsesPromptInput()explicitly optsscopeandforceout of prompt syncing, but this handler still writes those values intopromptwhen the user toggles or navigates onto them. That defeats the prompt/form split you added for this form and leaves misleading text in the composer.🤖 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 7676 - 7731, When updating lane-delete non-text controls, stop writing their values into the composer: remove or replace setPrompt calls for the "scope" and "force" toggle handlers so they do not set the prompt (e.g., call setPrompt("") or skip calling it). Also change the field navigation logic that currently does setPrompt(fields[nextIndex] ? nextValues[fields[nextIndex]!.name] ?? "" : "") to only populate the prompt when formFieldUsesPromptInput(fields[nextIndex]) returns true; otherwise clear or leave the prompt empty. Update the handlers referencing scope, force, setFormValues, currentFormValues, setFormFieldIndex and the navigation code in the rightPane form block accordingly.
🧹 Nitpick comments (6)
apps/ade-cli/src/tuiClient/components/TerminalPane.tsx (1)
106-134: ⚡ Quick winRename the newly introduced constants to camelCase.
The new constant identifiers in this block violate 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.🤖 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/components/TerminalPane.tsx` around lines 106 - 134, The new constants (CLAUDE_SPINNER_RE, CLAUDE_SPINNER_STATUS_WORDS, CLAUDE_SPINNER_STATUS_RE, CLAUDE_USAGE_STATUS_RE, TERMINAL_BOX_CHROME_RE, CLAUDE_INPUT_CHROME_RE, CLAUDE_PROMPT_CHROME_RE, CLAUDE_FOOTER_CHROME_RE, CLAUDE_SESSION_CHROME_RE, CLAUDE_LOGO_RE) must be renamed to camelCase (e.g., claudeSpinnerRe, claudeSpinnerStatusWords, claudeSpinnerStatusRe, claudeUsageStatusRe, terminalBoxChromeRe, claudeInputChromeRe, claudePromptChromeRe, claudeFooterChromeRe, claudeSessionChromeRe, claudeLogoRe) and all internal references updated accordingly; ensure TypeScript exports/usage sites are adjusted and tests/other imports updated to the new identifiers to avoid breakage.apps/ade-cli/src/tuiClient/components/AdeWordmark.tsx (1)
9-10: ⚡ Quick winRename uppercase identifiers to camelCase to match repo TS/JS naming rule.
FACE_ROWS,COMPACT_ROWS,ADE_WORDMARK_FULL_WIDTH, andADE_WORDMARK_COMPACT_WIDTHviolate the naming convention configured for TS/JS variables in this repo.Suggested rename
-const FACE_ROWS = [ +const faceRows = [ ... -const COMPACT_ROWS = [ +const compactRows = [ ... -export const ADE_WORDMARK_FULL_WIDTH = 36; -export const ADE_WORDMARK_COMPACT_WIDTH = 26; +export const adeWordmarkFullWidth = 36; +export const adeWordmarkCompactWidth = 26; ... - const rows = compact ? COMPACT_ROWS : FACE_ROWS; + const rows = compact ? compactRows : faceRows;As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.Also applies to: 28-29, 37-38, 41-41
🤖 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/components/AdeWordmark.tsx` around lines 9 - 10, Rename the uppercase constants to camelCase to follow the repo naming rule: change FACE_ROWS -> faceRows, COMPACT_ROWS -> compactRows, ADE_WORDMARK_FULL_WIDTH -> adeWordmarkFullWidth, and ADE_WORDMARK_COMPACT_WIDTH -> adeWordmarkCompactWidth; update all usages in AdeWordmark.tsx (and any imports/exports) to the new names and run a quick search to replace any remaining references to the old identifiers so TypeScript/JS builds remain clean.apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.ts (1)
109-111: 🏗️ Heavy liftStartup errors on the bridge socket are only logged; await
server.listen()to fail fast.The
server.listen()call on line 109 is asynchronous but not awaited. If binding fails (permission error, port conflict, etc.), the function returns successfully while the error handler only logs the failure. The daemon's lazy connection design (desktopBridgeClient.tschecksfs.existsSync()and retries on failure) masks this issue in practice, but the server implementation should properly expose startup errors rather than silently logging them.Make
startBuiltInBrowserDesktopBridgeServer()async and await the listen completion so bind failures propagate to the caller, enabling proper error handling at startup.🤖 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/builtInBrowser/desktopBridgeServer.ts` around lines 109 - 111, The startup currently calls server.listen(socketPath, ...) without awaiting so bind errors are only logged; update startBuiltInBrowserDesktopBridgeServer to be async and await server.listen by wrapping it in a Promise that resolves on the server's "listening" event (where you keep the logger.info("built_in_browser_bridge.listening", { socketPath })) and rejects on the server's "error" event so bind failures propagate to the caller; ensure the Promise cleans up listeners on resolve/reject and that the function signature and callers are adjusted to await the async startBuiltInBrowserDesktopBridgeServer.apps/desktop/src/main/services/chat/agentChatService.ts (1)
20389-20400: 💤 Low valueRedundant conditional could be simplified.
The ternary on lines 20394-20396 evaluates to
group.key === providerin both branches since whenprovider === "droid", checkinggroup.key === "droid"is equivalent togroup.key === provider.♻️ Suggested simplification
const modelCatalogContainsRefreshProvider = ( catalog: AgentChatModelCatalog, provider: AgentChatModelCatalogRefreshProvider, ): boolean => { return (catalog.groups ?? []).some((group) => { - const groupMatches = provider === "droid" - ? group.key === "droid" - : group.key === provider; - if (!groupMatches) return false; + if (group.key !== provider) return false; return (group.providers ?? []).some((entry) => entry.modelCount > 0); }); };🤖 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 20389 - 20400, The function modelCatalogContainsRefreshProvider contains a redundant ternary comparing provider === "droid" to decide between group.key === "droid" and group.key === provider; simplify by directly comparing group.key === provider and remove the ternary. Update the closure inside the (catalog.groups ?? []).some call (referencing group.key, provider, group.providers and entry.modelCount) so it only checks group.key === provider and then returns (group.providers ?? []).some(entry => entry.modelCount > 0).apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsx (1)
136-140: 💤 Low valueMagic number for subProvider indentation.
The
marginLeft={6}creates visual indentation for the subProvider text, but the value is hard-coded and doesn't clearly relate to the layout calculation. Consider deriving it from the actual glyph and label indentation (e.g.,2 spaces + star + space + glyph + space = 6) or extracting it as a named constant for clarity.🤖 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/components/ModelPicker/ModelPickerPane.tsx` around lines 136 - 140, The hard-coded indentation marginLeft={6} in ModelPickerPane (rendering entry.subProvider) is a magic number; replace it with a named constant or compute it from the layout components (e.g., sum of "2 spaces + star + space + glyph + space") so the relationship to glyph/label spacing is explicit, and update the JSX to use that constant (referencing ModelPickerPane and entry.subProvider) so future changes to glyph/label widths remain consistent and the intent is clear.apps/ade-cli/src/tuiClient/components/RightPane.tsx (1)
154-166: ⚡ Quick winConsider using
intentfield instead of label text matching.The
actionGlyphColorfunction relies on exact label string matches ("new chat", "delete lane", etc.) to determine glyph colors. If any label text changes inLANE_DETAIL_ACTIONS, the color mapping will silently break. Since you've already added anintentfield (e.g.,"rescue-unstaged"), consider using it as the primary discriminator for semantic coloring, with label matching as a fallback.♻️ Refactored color selection using intent field
-function actionGlyphColor(label: string): string { - if (label === "new chat" || label === "stage all" || label === "commit" || label === "push") { +function actionGlyphColor(action: { label: string; intent?: string }): string { + if (action.intent === "rescue-unstaged") return theme.color.attention; + if (action.label === "delete lane") return theme.color.error; + if (action.label === "new chat" || action.label === "stage all" || action.label === "commit" || action.label === "push") { return theme.color.running; } - if (label === "delete lane") return theme.color.error; - if (label === "move unstaged to new lane") return theme.color.attention; - // open/create PR, diff, reparent → navigational return theme.color.violet; }Then update the call site:
function ActionRow({ k, label, detail, glyph, + intent, selected, width, }: { k: string; label: string; detail?: string; glyph?: string; + intent?: string; selected?: boolean; width: number; }) { const glyphChar = glyph ?? " "; - const glyphColor = actionGlyphColor(label); + const glyphColor = actionGlyphColor({ label, intent });🤖 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/components/RightPane.tsx` around lines 154 - 166, actionGlyphColor currently matches exact label strings which is brittle; change it to prefer an action.intent field (e.g., "rescue-unstaged", "destructive", "additive", "navigational") as the primary discriminator and fall back to the existing label text checks only if intent is missing, update the function signature (or add a thin wrapper) to accept the action object or an explicit intent string, map intents to theme.color.running / theme.color.error / theme.color.attention / theme.color.violet accordingly, and update all call sites (where LANE_DETAIL_ACTIONS are used) to pass the action or its intent so label-only matches remain a fallback.
🤖 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/services/builtInBrowser/desktopBridgeClient.ts`:
- Around line 75-87: In ensureClient(), prevent resurrecting a disposed client
by checking the disposed flag at the start of ensureClient() and again inside
the connect().then(...) callback before assigning to client; if disposed is
true, throw an error instead of returning/assigning; also move the connecting =
null reset into a .finally() on the connect() promise so connecting is cleared
regardless of success or failure. Ensure references to connecting, client,
disposed, connect(), and ensureClient() are used so the checks and throws are
applied in both the initial entry and inside the .then() handler.
- Around line 66-72: The timeout Promise used in both JsonRpcClient.connect (the
connection logic around JsonRpcClient.connect(socketPath)) and callBridge should
be refactored into a small helper (e.g., runWithTimeout or raceWithTimeout) that
creates the setTimeout, returns the raced Promise, and ensures the timeout is
cleared in a finally block; update the connect() usage and callBridge() usage to
call this helper instead of creating an unresolved timer Promise so that the
timeout ID is captured and clearTimeout(timeoutId) is called after the race
settles.
In `@apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx`:
- Around line 186-187: The test currently asserts ordering via indexOf on the
string variable frame (expect(frame.indexOf("explore-code")).toBeLessThan(...))
but doesn't ensure the sections exist first; modify the assertions to first
verify presence (e.g., expect(frame).toContain("explore-code"),
expect(frame).toContain("background"), expect(frame).toContain("dev:desktop") or
check indexOf(...) !== -1) and only then assert ordering using
frame.indexOf("explore-code") < frame.indexOf("background") and
frame.indexOf("background") < frame.indexOf("dev:desktop") so the test fails
clearly if a section is missing; update the assertions in RightPane.test.tsx
around the frame index checks accordingly.
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 3730-3796: The form-opening handlers (openMoveUnstagedForm and
openLaneDeleteForm) currently rely on activeLaneIdRef.current at submit time, so
capture the current lane into the form payload when opening the form and use
that captured identifier at submit; specifically, when calling openForm from
openMoveUnstagedForm add a stable identifier (e.g., laneId or branchRef and
lane.name) into the form descriptor so the submit path (commands
"new-lane-from-unstaged" and "lane-delete" / laneDelete payload) consumes that
captured value instead of re-reading activeLaneIdRef.current; update the
openForm payload fields for openLaneDeleteForm to include the immutable
laneId/branchRef and for openMoveUnstagedForm include laneId/branchRef so
downstream handlers use those stored values to operate on the intended lane.
- Around line 1770-1786: The promptHitLine function is being called with only
rows and promptRowCount, causing hit-testing to ignore the actual rendered
modelStatusRows/footerRows; update the click handler(s) that call promptHitLine
(and the other identical call sites referenced) to pass the current computed
modelStatusRows and footerRows values from the rendered layout (the same values
used to render the status/footer) instead of relying on defaults, or
alternatively change promptHitLine to accept the layout object and extract
promptRowCount, modelStatusRows, and footerRows from it; ensure you update every
call site (including the duplicated usage noted) so clicks near the bottom use
the live rendered row counts rather than defaults.
- Around line 1592-1596: The mouse-to-field index mapping for the "lane-delete"
branch in app.tsx returns indices in the wrong order (scope, remoteName, force,
confirm) causing inconsistency with keyboard navigation (scope, force,
remoteName, confirm); swap the return values for the y ranges so they match the
declared field order (scope => 0, force => 1, remoteName => 2, confirm => 3) by
changing the y 13-14 branch to return 1 and the y 16-17 branch to return 2, and
make the same change in the other "lane-delete" mapping block referenced (around
the other occurrence).
In `@apps/desktop/src/main/main.ts`:
- Around line 1088-1097: The call to startBuiltInBrowserDesktopBridgeServer can
throw and currently runs unguarded during app boot (using
builtInBrowserBridgeLogger and assigning builtInBrowserBridgeServer), so
surround the startup in a try/catch: call startBuiltInBrowserDesktopBridgeServer
inside a try block, assign its result to builtInBrowserBridgeServer on success,
and on error catch and log the failure via builtInBrowserBridgeLogger.error (or
process logger) and set builtInBrowserBridgeServer to null/undefined so the rest
of whenReady can continue without throwing; ensure any downstream code checks
the presence of builtInBrowserBridgeServer before using it.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Line 151: LAST_LAUNCH_CONFIG_KEY is currently a global key and
LastLaunchConfig payload is applied across contexts, allowing a launch config
with elevated permissions to be restored in the wrong project/lane/surface;
change the persistence so the key is scoped by context (for example append
projectId/laneId/surface to LAST_LAUNCH_CONFIG_KEY) or embed explicit scope
fields into LastLaunchConfig and validate them before applying in functions that
read/write this key (references: LAST_LAUNCH_CONFIG_KEY and LastLaunchConfig and
the read/write code paths around them); ensure writes include the current
context, reads verify the stored context matches current context, and refuse to
auto-apply if the scope does not match.
- Around line 3572-3577: The current logic incorrectly gives precedence to the
stored config from readLastLaunchConfig over the freshly built
latestSessionConfig; change the precedence so latestSessionConfig is used first:
compute latestSessionConfig via buildLastLaunchConfig(sessions[0],
initialNativeControls) and then set config to latestSessionConfig ??
readLastLaunchConfig(initialNativeControls) (or null if neither), then guard and
call applyLaunchConfigToComposer(config); update references to
latestSessionConfig, readLastLaunchConfig, buildLastLaunchConfig,
applyLaunchConfigToComposer, sessions, and initialNativeControls accordingly.
In `@apps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsx`:
- Around line 281-303: The attachment chip currently uses tabIndex={-1} which
prevents keyboard users reaching the element to trigger onKeyDown handlers (see
ChatAttachmentPendingImage usage and handleImageAttachmentKeyDown invocation);
change the focus strategy so chips are keyboard reachable—either make the outer
element tabbable (tabIndex={0}) or replace/augment it with an inner semantic
control (e.g., a button or an element with role="button" and tabIndex={0}) and
wire the same onKeyDown/onClick handlers (ensure onRemove and onFocusPrompt are
preserved), so that arrow/delete key handling and focus management work for
keyboard-only users.
In `@scripts/tui-web.mjs`:
- Around line 504-511: In rememberPtyOutput, a single large chunk can exceed
ptyReplayLimitBytes because the loop only runs while ptyReplayChunks.length > 1;
change the logic to enforce a hard cap before or when enqueueing: compute
newChunkSize = Buffer.byteLength(text) and if newChunkSize >=
ptyReplayLimitBytes trim the chunk to the last ptyReplayLimitBytes bytes (or
split it) so its size never alone exceeds ptyReplayLimitBytes, then push and
update ptyReplayBytes; otherwise push and then remove oldest chunks in the while
loop until ptyReplayBytes <= ptyReplayLimitBytes (without requiring length > 1).
Ensure you update ptyReplayBytes consistently when trimming/splitting.
---
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 6265-6274: The background launch call to
startClaudeTerminalSession is missing the reasoningEffort param and thus drops
the user's selected effort; update the call site in app.tsx (the
startClaudeTerminalSession invocation) to pass reasoningEffort:
normalized.reasoningEffort (or the appropriate field on normalized that holds
the current effort) — matching the foreground path used by
startClaudeTerminalForPrompt — so the background-launched Claude session
receives the same reasoningEffort value (use a fallback/default if needed).
- Around line 7676-7731: When updating lane-delete non-text controls, stop
writing their values into the composer: remove or replace setPrompt calls for
the "scope" and "force" toggle handlers so they do not set the prompt (e.g.,
call setPrompt("") or skip calling it). Also change the field navigation logic
that currently does setPrompt(fields[nextIndex] ?
nextValues[fields[nextIndex]!.name] ?? "" : "") to only populate the prompt when
formFieldUsesPromptInput(fields[nextIndex]) returns true; otherwise clear or
leave the prompt empty. Update the handlers referencing scope, force,
setFormValues, currentFormValues, setFormFieldIndex and the navigation code in
the rightPane form block accordingly.
In `@apps/desktop/src/shared/chatSubagents.ts`:
- Around line 235-250: incomingTaskType is currently any string and gets
force-cast into SubagentSnapshot.taskType; validate it against the allowed
literals ("subagent" | "background" | "local_workflow" | "cron" | "other")
before assignment (e.g., implement an isValidTaskType(value) type guard or
whitelist array) and only include taskType in the base object if
incomingTaskType passes that check (or fall back to existing?.taskType if that
is already valid); update the logic around incomingTaskType, the base object
construction, and any places using taskType to rely on the validated value so
invalid strings are never force-cast into SubagentSnapshot.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/components/AdeWordmark.tsx`:
- Around line 9-10: Rename the uppercase constants to camelCase to follow the
repo naming rule: change FACE_ROWS -> faceRows, COMPACT_ROWS -> compactRows,
ADE_WORDMARK_FULL_WIDTH -> adeWordmarkFullWidth, and ADE_WORDMARK_COMPACT_WIDTH
-> adeWordmarkCompactWidth; update all usages in AdeWordmark.tsx (and any
imports/exports) to the new names and run a quick search to replace any
remaining references to the old identifiers so TypeScript/JS builds remain
clean.
In `@apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsx`:
- Around line 136-140: The hard-coded indentation marginLeft={6} in
ModelPickerPane (rendering entry.subProvider) is a magic number; replace it with
a named constant or compute it from the layout components (e.g., sum of "2
spaces + star + space + glyph + space") so the relationship to glyph/label
spacing is explicit, and update the JSX to use that constant (referencing
ModelPickerPane and entry.subProvider) so future changes to glyph/label widths
remain consistent and the intent is clear.
In `@apps/ade-cli/src/tuiClient/components/RightPane.tsx`:
- Around line 154-166: actionGlyphColor currently matches exact label strings
which is brittle; change it to prefer an action.intent field (e.g.,
"rescue-unstaged", "destructive", "additive", "navigational") as the primary
discriminator and fall back to the existing label text checks only if intent is
missing, update the function signature (or add a thin wrapper) to accept the
action object or an explicit intent string, map intents to theme.color.running /
theme.color.error / theme.color.attention / theme.color.violet accordingly, and
update all call sites (where LANE_DETAIL_ACTIONS are used) to pass the action or
its intent so label-only matches remain a fallback.
In `@apps/ade-cli/src/tuiClient/components/TerminalPane.tsx`:
- Around line 106-134: The new constants (CLAUDE_SPINNER_RE,
CLAUDE_SPINNER_STATUS_WORDS, CLAUDE_SPINNER_STATUS_RE, CLAUDE_USAGE_STATUS_RE,
TERMINAL_BOX_CHROME_RE, CLAUDE_INPUT_CHROME_RE, CLAUDE_PROMPT_CHROME_RE,
CLAUDE_FOOTER_CHROME_RE, CLAUDE_SESSION_CHROME_RE, CLAUDE_LOGO_RE) must be
renamed to camelCase (e.g., claudeSpinnerRe, claudeSpinnerStatusWords,
claudeSpinnerStatusRe, claudeUsageStatusRe, terminalBoxChromeRe,
claudeInputChromeRe, claudePromptChromeRe, claudeFooterChromeRe,
claudeSessionChromeRe, claudeLogoRe) and all internal references updated
accordingly; ensure TypeScript exports/usage sites are adjusted and tests/other
imports updated to the new identifiers to avoid breakage.
In `@apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.ts`:
- Around line 109-111: The startup currently calls server.listen(socketPath,
...) without awaiting so bind errors are only logged; update
startBuiltInBrowserDesktopBridgeServer to be async and await server.listen by
wrapping it in a Promise that resolves on the server's "listening" event (where
you keep the logger.info("built_in_browser_bridge.listening", { socketPath }))
and rejects on the server's "error" event so bind failures propagate to the
caller; ensure the Promise cleans up listeners on resolve/reject and that the
function signature and callers are adjusted to await the async
startBuiltInBrowserDesktopBridgeServer.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 20389-20400: The function modelCatalogContainsRefreshProvider
contains a redundant ternary comparing provider === "droid" to decide between
group.key === "droid" and group.key === provider; simplify by directly comparing
group.key === provider and remove the ternary. Update the closure inside the
(catalog.groups ?? []).some call (referencing group.key, provider,
group.providers and entry.modelCount) so it only checks group.key === provider
and then returns (group.providers ?? []).some(entry => entry.modelCount > 0).
🪄 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: 51114a14-d76b-485d-9adf-abaa32e76add
⛔ Files ignored due to path filters (8)
README.mdis excluded by!*.mddocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/missions/README.mdis excluded by!docs/**docs/features/missions/orchestration.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (61)
apps/ade-cli/pnpm-workspace.yamlapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.test.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.tsapps/ade-cli/src/services/projects/machineLayout.test.tsapps/ade-cli/src/services/projects/machineLayout.tsapps/ade-cli/src/services/projects/projectRegistry.test.tsapps/ade-cli/src/services/projects/projectScope.test.tsapps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsxapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/TerminalPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/AdeWordmark.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/components/TerminalPane.tsxapps/ade-cli/src/tuiClient/linearCommands.tsapps/ade-cli/src/tuiClient/rightPaneFormatters.tsapps/ade-cli/src/tuiClient/types.tsapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/scripts/dev.cjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/remoteRuntime/remoteRuntime.offlineRpc.integration.test.tsapps/desktop/src/main/services/runtime/machineStateMigration.test.tsapps/desktop/src/main/services/runtime/machineStateMigration.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.test.tsxapps/desktop/src/renderer/components/chat/ChatAttachmentTray.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.tsxapps/desktop/src/renderer/components/chat/chatExecutionSummary.test.tsapps/desktop/src/renderer/components/chat/chatExecutionSummary.tsapps/desktop/src/renderer/components/missions/MissionArtifactsTab.tsxapps/desktop/src/renderer/components/missions/MissionChatV2.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.test.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.test.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPickerContent.tsxapps/desktop/src/renderer/components/shared/ModelPicker/providerEmptyState.test.tsxapps/desktop/src/renderer/components/shared/ModelPicker/providerEmptyState.tsxapps/desktop/src/renderer/components/shared/ModelPicker/runtimeCatalogCache.tsapps/desktop/src/shared/chatSubagents.tsapps/desktop/src/shared/types/chat.tsscripts/tui-web.mjs
💤 Files with no reviewable changes (1)
- apps/ade-cli/src/cli.ts
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Review-only pass completed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR delivers a TUI performance and functionality pass: responsive drawer width, lane-deletion with scope/force controls, draft config persistence across chat sessions, Claude terminal output filtering, and subagent/background-task categorization with visual refresh.
scope/forcefield routing,isTerminalSessionWorkingfor smarter spin-tick gating, andcompactClosedTerminalTranscriptto strip Claude UI chrome from closed terminal previews.LastLaunchConfigpersisted to localStorage so composer settings survive session creation;notifySessionCreatedmade synchronous to fix a new-chat-not-visible race; keyboard nav added to image attachment chips.taskType/workflowNamepropagated through the full task lifecycle; Cursor catalog staleness fixed (provider only marked fresh when models are actually returned);discovery-emptyempty-state mode for authed-but-empty providers.Confidence Score: 5/5
Safe to merge — changes are well-scoped UI and service logic with no data-loss paths; the noted issues are minor correctness and heuristic edge cases.
All functional logic (task lifecycle, config persistence, keyboard nav, terminal filtering) is guarded and tested. The missing
lastLaunchConfigStorageKeydependency would only misbehave ifprojectRoot/surfaceProfile/workDraftKindchange while the component is mounted, which is not a typical lifecycle transition. Theprocess.killin the render body and the 4-letter word filter are quality concerns, not correctness blockers.AgentChatPane.tsx(stale closure inlaunchShellForDraftLane) andTerminalPane.tsx(isShortClaudeSpinnerResidueover-broad filter) are worth a second look before the next iteration.Important Files Changed
isTerminalSessionWorking(usesprocess.killin render path), andresolveModelPickerEscape. Spin-tick logic simplified to only animate when drawer is open.compactClosedTerminalTranscriptto strip Claude UI chrome from closed terminal previews. TheisShortClaudeSpinnerResidueheuristic can false-positive on 1–4 letter real output words when 4+ appear.LastLaunchConfigpersistence to localStorage so draft composer settings survive session creation;notifySessionCreatedchanged from microtask-deferred to synchronous to fix race condition. MissinglastLaunchConfigStorageKeydep inlaunchShellForDraftLane.task_type/workflow_namepropagation through task lifecycle events; suppresses ambientskip_transcripttasks; fixes Cursor catalog staleness by requiring the catalog to actually contain Cursor models before marking the provider fresh.forwardRef, adds keyboard navigation (arrow keys, Delete/Backspace) for image attachments, and propagatesonFocusPromptcallback through all image chip variants.MinusCirclefor stopped state, section dot indicators, andworkflowNamedisplay.taskTypeandworkflowNamefields toSubagentSnapshotwith normalization helpers; both fields are merged from incoming events over existing state.providerAuthStatustoEmptyStateand firesonProviderRailSelectwhen re-clicking the currently selected rail, enabling adiscovery-emptystate when a provider is authed but returns no models.shouldMarkRefreshProviderFreshguard so a Cursor catalog refresh is only marked fresh when the resulting catalog actually contains Cursor models, preventing premature cache hits.widthprop for responsive sizing (capped atDRAWER_WIDTH_MAX = 48), adjustsvisibleDrawerLaneCountformula to use 5 rows/card instead of 4.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Session Created / Session Selected] --> B{Has session?} B -- No --> C{readLastLaunchConfig from localStorage} C -- found --> D[applyLaunchConfigToComposer] C -- not found --> E[Use initialNativeControls defaults] B -- Yes --> F[syncComposerToSession: load session settings] F --> D G[launchShellForDraftLane] --> H[createChatSession API] H --> I[buildLastLaunchConfig] I --> J[writeLastLaunchConfig to localStorage] J --> K[notifySessionCreated - synchronous] subgraph TUI spin-tick L[activeTerminalSession != null] --> M{isTerminalSessionWorking} M -- process.kill pid 0 --> N[spinTickActive = true] M -- pid dead / idle --> O[spinTickActive = false] P[drawerOpen && sessions animating] --> N endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address pr loop feedback" | Re-trigger Greptile