Tui And Desktop App Upgrades#291
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds a plan-mode skill spec and broad feature work across CLI/TUI, desktop, and iOS: a block-based TUI rendering/aggregation pipeline, provider/chat-service enhancements (Claude/Codex/Cursor), Linear issue-attachment and lane-card publishing, usage threshold detection + UI toasts, resume-target sanitization and Codex resume improvements, credential-store migrations, lane diff/PR summaries, many tests, and release artifact validation. ChangesTUI rendering, aggregation, and related types
Chat services, provider integrations, slash/steer APIs
Linear issue attachments, lane publishing, diff/PR summaries
Usage tracking, threshold events, IPC and UI
Resume-target sanitization, Codex resume, and PTY/session changes
Credential-store migration & API key handling
iOS sync, project catalog, timeouts, and database
Release workflow & minor infra
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
@cursor review |
|
@copilot review |
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c10870a. Configure here.
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/usage/UsageQuotaPanel.tsx (1)
134-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent stale overwrite from concurrent initial fetches.
Line 135 (
load) and Line 144 (manualRefresh) run in parallel; whichever resolves last wins. A slowergetSnapshot()can overwrite freshly refreshed state.Suggested fix
useEffect(() => { - void load(); + let cancelled = false; + void (async () => { + await load(); + if (!cancelled) await manualRefresh(); + })(); if (!window.ade?.usage) return; const unsubscribe = window.ade.usage.onUpdate(applySnapshot); - return unsubscribe; -}, [applySnapshot, load]); - -// Auto-refresh on open so the drawer always shows fresh data instead of -// whatever the last background poll happened to leave behind. -useEffect(() => { - void manualRefresh(); -}, [manualRefresh]); + return () => { + cancelled = true; + unsubscribe(); + }; +}, [applySnapshot, load, manualRefresh]);As per coding guidelines, Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
🤖 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/usage/UsageQuotaPanel.tsx` around lines 134 - 145, Both useEffect calls can race: load() and manualRefresh() may resolve out of order and the slower one can overwrite newer state; update load and manualRefresh to use a shared increasing requestId (or AbortController) stored in component scope, increment before each async getSnapshot call, capture the current id in the closure, and only call applySnapshot if the captured id equals the latest id (i.e., request is still current); reference the existing symbols load, manualRefresh, and applySnapshot when adding the guard so only the most recent fetch result is applied.
🟡 Minor comments (13)
apps/desktop/package.json-91-91 (1)
91-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused
mdast-util-find-and-replacedependency.This dependency is not imported or referenced anywhere in the codebase. Adding unused packages bloats the Electron app bundle; it should be removed unless it's legitimately needed for this PR.
🤖 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/package.json` at line 91, Remove the unused dependency "mdast-util-find-and-replace" from the package.json dependencies block (the entry shown in the diff) and update the lockfile by running the package manager install (e.g., npm/yarn/pnpm install) so the lockfile and node_modules reflect the removal; verify no imports reference mdast-util-find-and-replace and then rebuild the Electron app to ensure bundle size is reduced.apps/desktop/src/main/services/sync/syncHostService.test.ts-15-21 (1)
15-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
spawnMockbetween tests to prevent cross-test leakage.
spawnMockis declared at module scope but never reset, causing call history and implementations to bleed into later tests. The implementation usesspawn()at line 1287 ofapps/ade-cli/src/services/sync/syncHostService.ts, so this mock needs cleanup alongsideexecFileMock.Proposed fix
afterEach(async () => { while (activeDisposers.length > 0) { const dispose = activeDisposers.pop(); if (dispose) await dispose(); } execFileMock.mockReset(); + spawnMock.mockReset(); });🤖 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/sync/syncHostService.test.ts` around lines 15 - 21, The module-scoped spawnMock retains calls/implementation across tests; add cleanup for spawnMock alongside execFileMock in the test teardown (e.g., afterEach) by calling spawnMock.mockReset() or spawnMock.mockClear() (or use vi.clearAllMocks()/vi.resetAllMocks()) so the mocked spawn used by spawn() does not leak between tests; reference the existing spawnMock and execFileMock symbols and ensure this cleanup runs afterEach test.apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts-195-217 (1)
195-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack terminal-first task updates before emitting the result.
If the first
"task"frame for arun_idalready has a terminal status, this branch emitssubagent_resultwithout recording that state. A duplicated/replayed terminal frame for the same run will emit another result becauseprevStatusis stillnull.🤖 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/cursorSdkEventMapper.ts` around lines 195 - 217, When handling a first-seen task frame (prevStatus === null) that already has a terminal status, record that terminal state in meta.taskStatusMap for runId before emitting the result so future duplicate/replayed terminal frames won't emit another result; specifically, in the branch inside the if (runId) block where prevStatus === null && isCursorTaskTerminalStatus(status), call meta.taskStatusMap.set(runId, status) prior to out.push(makeResultEvent(status)) (leave other branches using tagRuntime and non-terminal logic unchanged).apps/desktop/src/main/services/chat/claudeOutputStyles.ts-104-106 (1)
104-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
pluginKeyName()drops scoped plugin names.
split("@", 1)returns""for keys like@scope/plugin@1.2.3, soenabledPluginNames()drops them and local plugin discovery can never match the enabled plugin.💡 Suggested fix
function pluginKeyName(pluginKey: string): string { - return pluginKey.split("@", 1)[0]?.trim().toLowerCase() ?? ""; + const trimmed = pluginKey.trim(); + const versionSeparator = trimmed.lastIndexOf("@"); + const base = versionSeparator > 0 ? trimmed.slice(0, versionSeparator) : trimmed; + return base.toLowerCase(); }🤖 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/claudeOutputStyles.ts` around lines 104 - 106, pluginKeyName() currently uses split("@", 1) which returns an empty string for scoped names like `@scope/plugin`@1.2.3 and causes enabledPluginNames() to drop them; change pluginKeyName to strip only the version suffix by finding the last "@" and returning the substring before it (but if the last "@" is at index 0 or not present, return the whole key trimmed/lowercased) so scoped names like `@scope/plugin` are preserved and matching in enabledPluginNames() works correctly.apps/desktop/src/renderer/components/app/TopBar.tsx-92-99 (1)
92-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider deduplicating forced refreshes.
When
force: trueis passed, line 99 bypasses the in-flight request check. If multiple components or event handlers callfetchRecent({ force: true })simultaneously (e.g., both the focus handler and the missing-project event), you'll create duplicate concurrent requests instead of sharing a single forced fetch.🔒 Proposed fix to deduplicate forced refreshes
- if (!options?.force && recentProjectsInFlight) return recentProjectsInFlight; + if (recentProjectsInFlight && !options?.force) return recentProjectsInFlight; + // Even when forced, if a fetch is already in flight, wait for it rather than starting another + if (recentProjectsInFlight) return recentProjectsInFlight;Alternatively, if you truly need force to always start a fresh request:
if ( !options?.force && recentProjectsCache && now - recentProjectsCache.fetchedAtMs < RECENT_PROJECTS_CACHE_TTL_MS ) { return Promise.resolve(recentProjectsCache.rows); } - if (!options?.force && recentProjectsInFlight) return recentProjectsInFlight; + // Cancel the previous in-flight request when forced, or dedupe if not forced + if (recentProjectsInFlight) { + if (options?.force) { + // Mark the old promise stale; callers will get newer data from the next fetch + } else { + return recentProjectsInFlight; + } + }The simpler fix is to always dedupe, even on force, since the cache is already short-lived (2.5s TTL).
🤖 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/app/TopBar.tsx` around lines 92 - 99, The fetchRecent logic allows duplicate concurrent requests when options.force is true because the in-flight check is skipped; modify fetchRecent so that recentProjectsInFlight is always consulted before starting a new fetch (i.e., return recentProjectsInFlight if set regardless of options.force) and ensure recentProjectsInFlight is set immediately when initiating the network call (and cleared on completion/error) so forced refreshes are deduplicated; update references in fetchRecent, recentProjectsInFlight, options.force, recentProjectsCache, and RECENT_PROJECTS_CACHE_TTL_MS accordingly.apps/desktop/src/main/services/cto/linearLaneCardService.ts-44-46 (1)
44-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an inline comment explaining the
{linkedAt__since}Linear template variable.Line 45 intentionally uses
{linkedAt__since}as a Linear API template placeholder. The actuallinkedAttimestamp is provided in metadata.linkedAt for Linear to process. Add a comment explaining that Linear replaces this placeholder with a "time since" formatted string at render time:Suggested documentation
return { issueId: args.issue.id, title: `ADE lane: ${truncate(args.lane.name, 64)}`, + // {linkedAt__since} is replaced by Linear with "linked X time ago" formatting subtitle: `${truncate(branch, 56)} - linked {linkedAt__since}`, url: buildCardUrl(args.issue, args.lane.id),🤖 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/cto/linearLaneCardService.ts` around lines 44 - 46, Add an inline comment next to the subtitle template showing that `{linkedAt__since}` is a Linear template placeholder which Linear will replace with a “time since” string at render time using the actual timestamp provided in metadata.linkedAt; update the file linearLaneCardService.ts near the subtitle assignment (where subtitle: `${truncate(branch, 56)} - linked {linkedAt__since}` is defined) to include a short comment referencing metadata.linkedAt so future readers know the placeholder is intentionally left for Linear to process.apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx-417-425 (1)
417-425:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard slash-command mock args before provider access
Line 418 dereferences
args.providerwithout guardingargs. If the call path passesundefined, this test throws a TypeError instead of asserting behavior.Proposed fix
- vi.mocked(window.ade.agentChat.slashCommands).mockImplementation(async (args) => { - if (args.provider === "claude") { + vi.mocked(window.ade.agentChat.slashCommands).mockImplementation(async (args) => { + if (args?.provider === "claude") { return [{ name: "/agents", description: "Manage agent configurations.", source: "sdk", }]; } return []; });🤖 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/chat/AgentChatPane.submit.test.tsx` around lines 417 - 425, The mock implementation for window.ade.agentChat.slashCommands dereferences args.provider unguarded; update the mock in AgentChatPane.submit.test.tsx so it safely handles undefined args (e.g., check that args is defined before accessing args.provider or use a default param) and only returns the Claude commands when the provider equals "claude"; ensure the mock returns [] for any undefined or non-claude args to avoid TypeError during tests.apps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsx-21-23 (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
stripAnsiregex is missing the ESC character.Real ANSI SGR sequences start with
\x1B[(or sometimes\x9B), but this regex matches only[<digits>m. As-is it will leave an orphaned\x1Bbyte in the output and would also (in principle) strip any literal[<digits>msubstrings that happen to appear in legitimate content. Most of the suite gets away with this because the assertions use.toContain, but the helper is fragile and easy to misuse for negative assertions.🧹 Proposed fix
function stripAnsi(value: string): string { - return value.replace(/\[[0-9;]*m/g, ""); + // eslint-disable-next-line no-control-regex + return value.replace(/\x1B\[[0-9;]*m/g, ""); }🤖 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/__tests__/ChatView.test.tsx` around lines 21 - 23, The stripAnsi helper's regex is wrong because it doesn't include the ESC control byte, so it may leave an orphaned ESC or wrongly strip plain text; update the stripAnsi function to match real ANSI SGR sequences by including the ESC (and optionally 0x9B) prefix — e.g., change the regex used in stripAnsi to match either \x1B or \x9B followed by '[' and the parameter bytes (digits and semicolons) and the final 'm' (for example use a pattern like (?:\x1B|\x9B)\[[0-9;]*m with the global flag) so it only removes real ANSI sequences.apps/desktop/src/main/services/chat/agentChatService.test.ts-1220-1236 (1)
1220-1236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a separator before
userTextin the VM context helper
userTextis concatenated directly onto the last bullet line, which creates malformed synthetic prompt text and can reduce test fidelity for delimiter-sensitive stripping logic.Suggested patch
function automaticMacosVmContextText(userText: string): string { return [ @@ - `- Keep VM-side edits inside the mounted guest lane path so the host lane and guest stay aligned.${userText}`, + "- Keep VM-side edits inside the mounted guest lane path so the host lane and guest stay aligned.", + userText, ].join("\n"); }🤖 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.test.ts` around lines 1220 - 1236, automaticMacosVmContextText currently concatenates userText directly onto the last bullet, corrupting delimiters; update automaticMacosVmContextText to insert a clear separator before appending userText (for example a newline or a distinct delimiter line such as "\n\n---\n" or at minimum "\n") so userText begins on its own line and does not merge with the preceding bullet.apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx-711-712 (1)
711-712:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHide the lane/tab-switch header actions when their callbacks are absent.
onOpenSessionInTabsViewandonGoToLaneare typed as optional (lines 635-636), but the "Open in Tabs view" and "Go to lane" buttons — plusLaneChip'sonClick={gotoLane}— render unconditionally. When the parent doesn't pass these props, the affordances are still presented as interactive but silently do nothing.Gate rendering on the presence of the callback (and pass a real
onClickonly when bound):🛠️ Proposed fix
- const openInTabs = () => onOpenSessionInTabsView?.(session.id); - const gotoLane = () => onGoToLane?.(session.laneId); + const openInTabs = onOpenSessionInTabsView ? () => onOpenSessionInTabsView(session.id) : null; + const gotoLane = onGoToLane ? () => onGoToLane(session.laneId) : null; return [session.id, { ... headerActions: ( <> <LaneChip laneName={session.laneName} laneColor={laneAccentColor} maxWidth={120} - onClick={gotoLane} + {...(gotoLane ? { onClick: gotoLane } : {})} /> ... - <SmartTooltip content={{ label: "Open in Tabs view", ... }}> - <button ... onClick={openInTabs} ...> - <ArrowSquareOut size={11} /> - </button> - </SmartTooltip> - <SmartTooltip content={{ label: "Go to lane", ... }}> - <button ... onClick={gotoLane} ...> - <Crosshair size={11} /> - </button> - </SmartTooltip> + {openInTabs ? ( + <SmartTooltip content={{ label: "Open in Tabs view", description: "Switch to the single-pane tab view focused on this session." }}> + <button type="button" onClick={openInTabs} ...><ArrowSquareOut size={11} /></button> + </SmartTooltip> + ) : null} + {gotoLane ? ( + <SmartTooltip content={{ label: "Go to lane", description: "Switch the active lane to this session's lane." }}> + <button type="button" onClick={gotoLane} ...><Crosshair size={11} /></button> + </SmartTooltip> + ) : null}Also applies to: 724-759
🤖 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` around lines 711 - 712, The "Open in Tabs view" and "Go to lane" affordances are rendered even when their callbacks are undefined; update the rendering logic so these buttons and LaneChip are only rendered when the corresponding props are present and only pass onClick handlers when bound: check onOpenSessionInTabsView before rendering the openInTabs button / creating openInTabs closure, and check onGoToLane before rendering the go-to-lane button and before passing gotoLane into LaneChip (i.e., render the elements conditionally and only supply onClick={gotoLane} when onGoToLane exists).apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx-982-1010 (1)
982-1010:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
role="tablist"to the ungrouped tab strip wrapper.
WorkTabrenders an inner<button role="tab">(line 507), but the ungrouped strip's wrapping<div className="ade-work-tab-strip-roomy ...">has norole="tablist". The grouped path already setsrole="tablist"on.ade-work-lane-band-tabs(line ~1148), so screen-reader semantics are inconsistent across tab modes and tab-group navigation breaks in the ungrouped path.♿ Proposed fix
- <div className="ade-work-tab-strip-roomy w-max min-w-0"> + <div className="ade-work-tab-strip-roomy w-max min-w-0" role="tablist"> {visibleSessions.map((session, index) => {🤖 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` around lines 982 - 1010, The ungrouped tab strip wrapper in WorkViewArea lacks the tablist role used by the grouped path; update the wrapping div that renders visibleSessions (the element with className "ade-work-tab-strip-roomy") to include role="tablist" so WorkTab's inner buttons (role="tab") are announced and keyboard navigation works consistently with the grouped path.apps/desktop/src/main/services/chat/agentChatService.ts-9891-9902 (1)
9891-9902:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSynthesize
subagent_startedonsession.deletedfor parity withsession.updated.The
session.updatedbranch callsensureSubagentStarted()first so the panel always has a row to update. The deleted branch skips that step, so ifsession.created/session.updatedwere missed (out-of-order delivery, late subscription, restart races) the renderer receives asubagent_resultfor ataskIdit never observed starting. Mirror the defensive call here to avoid orphan result rows.🛠️ Proposed fix
} else { // session.deleted + // Synthesize started first if we missed the created/updated events + // so the panel has a row to mark complete. + ensureSubagentStarted(); emitChatEvent(managed, { type: "subagent_result", taskId: childKey, parentToolUseId: null, status: "completed", summary: formatSummary(), turnId, }); runtime.subagentSessionIds.delete(childKey); }🤖 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 9891 - 9902, The session.deleted branch can emit a subagent_result for a taskId the renderer never saw start; call ensureSubagentStarted(...) before emitting so the panel always has a row to update. In the session.deleted branch (the block that currently calls emitChatEvent(managed, { type: "subagent_result", taskId: childKey, ... }) and runtime.subagentSessionIds.delete(childKey)), invoke the same ensureSubagentStarted(managed, childKey, turnId) call used in the session.updated branch (or its equivalent signature) immediately prior to emitChatEvent, then proceed to emit the subagent_result and delete from runtime.subagentSessionIds to maintain parity and avoid orphan result rows.apps/desktop/src/main/services/chat/agentChatService.ts-1303-1307 (1)
1303-1307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFragile coupling to producer wording — dedup silently regresses if upstream drifts.
stripAutomaticMacosVmContextPrefixdeduplicates the macOS VM context block by exact-matching one ofAUTOMATIC_MACOS_VM_CONTEXT_ENDINGS. These strings are produced byAgentChatPane.tsx(lines 434–436) and defined as static constants inagentChatService.ts(lines 1304–1306). If the renderer ever changes punctuation or wording in either ending,indexOfreturns-1on all iterations, the function returnsnull, and deduplication silently fails — the header reappears in subsequent messages.Consider exporting the header and endings from the producer module to establish a single source of truth, or add a unit test asserting the producer's output is recognized by
stripAutomaticMacosVmContextPrefix.🤖 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 1303 - 1307, stripAutomaticMacosVmContextPrefix is fragile because it exact-matches AUTOMATIC_MACOS_VM_CONTEXT_ENDINGS defined locally and duplicated in AgentChatPane.tsx; if the renderer wording drifts dedupe will silently fail. Fix by making the header and endings a single source of truth: move or export AUTOMATIC_MACOS_VM_CONTEXT_HEADER and AUTOMATIC_MACOS_VM_CONTEXT_ENDINGS from AgentChatPane.tsx (or a new shared module) and import them into agentChatService.ts so stripAutomaticMacosVmContextPrefix uses the same constants, and add a unit test that renders the producer output (AgentChatPane) and asserts stripAutomaticMacosVmContextPrefix successfully recognizes/deduplicates it.
🧹 Nitpick comments (20)
apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts (1)
132-132: 💤 Low valueConsider testing the
tasktool allowance more directly.The test now checks that
code_searchandweb_searchare disabled, which is correct. However, this doesn't directly verify the key change: that thetasktool is now allowed. Consider adding an assertion that explicitly confirmstaskis not in the disabled tools list, or add a comment explaining why checking other disabled tools is sufficient.Current approach is valid but less clear about the actual behavior change.
💡 Optional: Add explicit verification
"ade-plan": expect.objectContaining({ tools: expect.objectContaining({ code_search: false, web_search: false }), }), + // Verify task tool is not disabled (it should not be present in tools config) + "ade-plan": expect.objectContaining({ + tools: expect.not.objectContaining({ task: expect.anything() }), + }),Or simply add a comment:
"ade-plan": expect.objectContaining({ + // Verify code_search and web_search are disabled; task is intentionally omitted (allowed) tools: expect.objectContaining({ code_search: false, web_search: false }), }),🤖 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/opencode/openCodeRuntime.test.ts` at line 132, The test currently asserts tools: expect.objectContaining({ code_search: false, web_search: false }) but doesn't explicitly verify that the "task" tool is allowed; update the test in openCodeRuntime.test (the assertion block that sets "tools") to also explicitly assert the task tool is permitted—either by adding an expectation that "task" is true/allowed or by asserting that "task" is not present in the disabled list (or adding a clarifying comment next to tools: expect.objectContaining(...) explaining why checking other tools implies task is allowed). Locate the expectation that mentions "tools" and add the additional assertion referencing "task" so the key behavior change is directly tested.apps/desktop/src/main/services/ai/aiIntegrationService.test.ts (1)
455-463: ⚡ Quick winConsider adding positive assertions to strengthen test coverage.
The test correctly verifies that
probeClaudeRuntimeHealthis not called during forced refresh. However, it could be more robust by also asserting that:
- The status is returned successfully
- Claude availability is still correctly reported in the result
- The force refresh still works for other aspects (e.g., invalidates caches)
💚 Suggested enhancement
it("keeps forced status refresh non-interactive for Claude", async () => { const { service } = makeService({ availability: { claude: true, codex: false, cursor: false, droid: false }, }); - await service.getStatus({ force: true }); + const status = await service.getStatus({ force: true }); expect(mockState.probeClaudeRuntimeHealth).not.toHaveBeenCalled(); + expect(status.availableProviders.claude.binary.present).toBe(true); + expect(status.mode).toBe("subscription"); });This adds assertions confirming that the status is correctly populated even without the interactive probe.
🤖 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/ai/aiIntegrationService.test.ts` around lines 455 - 463, Add positive assertions to the "keeps forced status refresh non-interactive for Claude" test: call service.getStatus({ force: true }) and assert the returned status object is defined and indicates Claude is available (use the field on the returned object that represents Claude availability), also assert that forcing refresh invalidates/refreshes other cached aspects by comparing a prior cached status vs the new result or by calling getStatus without force then with force and verifying expected changes; reference makeService, service.getStatus and mockState.probeClaudeRuntimeHealth to locate where to add these checks.apps/ade-cli/src/services/sync/syncHostService.ts (1)
1299-1309: ⚡ Quick winAdd a fallback path when native
dns-sdpublish fails.On Line 1353, native mode becomes exclusive. On Line 1299 and Line 1305 failure/exit only logs, so LAN discovery can stay down indefinitely instead of falling back to Bonjour.
💡 Fallback sketch
+let nativeLanDiscoveryFailed = false; const shouldUseNativeLanDiscovery = (): boolean => - process.platform === "darwin" && typeof process.versions.electron === "string"; + process.platform === "darwin" + && typeof process.versions.electron === "string" + && !nativeLanDiscoveryFailed; const publishNativeLanDiscovery = ( serviceName: string, port: number, txt: Record<string, string>, ): void => { stopNativeLanDiscovery(); const child = spawn("dns-sd", [/* ... */], { stdio: "ignore" }); nativeBonjourProcess = child; child.unref(); child.once("error", (error) => { if (nativeBonjourProcess === child) nativeBonjourProcess = null; + nativeLanDiscoveryFailed = true; + publishLanDiscovery(port, { force: true }); args.logger.warn("sync_host.discovery_native_publish_failed", { error: error instanceof Error ? error.message : String(error), }); }); child.once("exit", (code, signal) => { if (nativeBonjourProcess !== child) return; nativeBonjourProcess = null; + nativeLanDiscoveryFailed = true; + publishLanDiscovery(port, { force: true }); args.logger.warn("sync_host.discovery_native_exited", { code, signal }); }); };Also applies to: 1353-1359
🤖 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/services/sync/syncHostService.ts` around lines 1299 - 1309, The error/exit handlers for the spawned native Bonjour process only log and clear nativeBonjourProcess, leaving LAN discovery down; update both child.once("error", ...) and child.once("exit", ...) to not only null out nativeBonjourProcess but also trigger a fallback to the built-in Bonjour path (e.g. call a function like startBonjourFallback() or enableBonjourMode()) whenever nativeBonjourProcess === child; also add the same fallback invocation where native mode is made exclusive (the block that flips native mode exclusive around the nativeBonjourProcess assignment) so that if native publish fails or native mode is exclusive and unusable, the code falls back to the non-native Bonjour implementation. Ensure the fallback function initializes the existing Bonjour discovery flow used elsewhere.apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts (1)
46-69: ⚡ Quick winConsider adding explicit codex-cli plan-mode test coverage.
The plan-mode tests now only exercise
codex-app-server, while the implementation treats bothcodex-cliandcodex-app-serveridentically (line 155 insystemPrompt.ts). While testing one runtime is sufficient for current behavior, adding a parallel test forcodex-cliwould make the test suite more robust and catch regressions if the conditional logic changes.🧪 Suggested test to add
it("keeps Codex CLI plan context aligned with native plan mode", () => { const result = buildCodingAgentSystemPrompt({ cwd: "/x", permissionMode: "plan", runtime: "codex-cli", }); expect(result).toContain("Native Codex Plan Mode controls planning and approval"); expect(result).toContain("proposed-plan mechanism"); expect(result).toContain("Do not use TodoWrite, update_plan, or exitPlanMode"); });🤖 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/ai/tools/systemPrompt.test.ts` around lines 46 - 69, Add parallel tests exercising runtime "codex-cli" to mirror the existing "codex-app-server" plan-mode cases: call buildCodingAgentSystemPrompt with permissionMode: "plan" and runtime: "codex-cli" and assert it contains "Native Codex Plan Mode controls planning and approval", "proposed-plan mechanism", and "Do not use TodoWrite, update_plan, or exitPlanMode"; also add the non-interactive variant (interactive: false) asserting it contains "make the safest reasonable assumptions" and does not contain "use request_user_input". Use the same test naming pattern as the existing "keeps Codex plan context aligned..." and "does not tell non-interactive..." tests so the new tests reference the same behavior paths.apps/desktop/src/main/utils/terminalSessionSignals.ts (1)
25-31: ⚡ Quick winConsider adding JSDoc documentation.
The validation logic is sound (control characters, leading dashes), but the function would benefit from JSDoc explaining:
- What constitutes a valid resume target ID
- Why control characters and leading "-" are rejected
- Return value semantics (null for invalid/empty vs. string for valid)
📝 Suggested documentation
+/** + * Validates and sanitizes a resume target ID for CLI commands. + * Rejects empty strings, control characters (including ANSI escapes), + * and values starting with "-" to prevent command injection. + * `@returns` Sanitized target ID, or null if invalid/empty + */ export function sanitizeResumeTargetId(value: string | null | undefined): string | null {🤖 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/utils/terminalSessionSignals.ts` around lines 25 - 31, Add JSDoc to sanitizeResumeTargetId describing what a valid resume target ID is (non-empty string without ASCII control characters [\x00-\x1F\x7F] and not starting with a hyphen), explain why control characters and leading "-" are rejected (sanitization/security/CLI semantics), and document the return semantics (returns null for empty or invalid inputs, otherwise the trimmed string). Place the JSDoc immediately above the sanitizeResumeTargetId declaration and reference the input type (string | null | undefined) and the exact return type (string | null) so callers understand expected behavior.apps/ade-cli/src/tuiClient/components/Header.tsx (1)
30-42: ⚡ Quick winConsider documenting the project name suppression logic.
The logic that hides the project name when it appears in the branch or worktree name is subtle:
- It checks if
branchRefends with the normalized project name (case-insensitive)- It checks if the worktree directory name exactly equals the normalized project name
This could be surprising behavior. For example, if your project is "auth" and your branch is "feature/oauth", the logic won't suppress "auth" even though "oauth" contains "auth" as a substring (because it uses
endsWith, notincludes).📝 Suggested documentation or extraction
Add a comment explaining the suppression rules:
+ // Hide the project name in the header if: + // 1. The project is "ade" (default/meta project name), OR + // 2. The branch ref already ends with the project name (e.g., branch="feat/myapp", project="myapp"), OR + // 3. The worktree directory name matches the project name exactly const projectRepeatsBranch = Boolean( normalizedProject && lane && ( branchRef.toLowerCase().endsWith(normalizedProject) || worktreeName === normalizedProject ), );Or extract to a helper function for clarity:
function shouldSuppressProjectName( projectName: string, lane: LaneSummary | null, ): boolean { const normalized = projectName.trim().toLowerCase(); if (!normalized || normalized === "ade") return true; if (!lane) return false; const branch = lane.branchRef?.trim().toLowerCase() ?? ""; const worktree = lane.worktreePath?.split(/[\\/]/).filter(Boolean).pop()?.toLowerCase() ?? ""; return branch.endsWith(normalized) || worktree === normalized; }🤖 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/Header.tsx` around lines 30 - 42, The current suppression logic (variables normalizedProject, branchRef, worktreeName, projectRepeatsBranch, showProject) is subtle and should be documented or extracted; update Header.tsx by either adding a concise comment above the block describing the rules (case-insensitive trim, branch must endWith the project name, worktree must equal the project dir, and "ade" is always hidden) or refactor the logic into a small helper function (e.g. shouldSuppressProjectName(projectName, lane)) that returns the Boolean now computed by projectRepeatsBranch/showProject and call that helper so the behavior is explicit and easier to test.apps/desktop/src/main/services/cto/linearCredentialService.ts (1)
270-308: ⚖️ Poor tradeoffConsider extracting migration steps for readability.
The
migrateLegacyProjectCredentialsIfNeededfunction handles multiple migration paths in a single 38-line function with nested conditionals. While the logic appears correct, it's challenging to verify all branches and could be fragile during future changes.♻️ Optional refactor to extract migration steps
const migrateLegacyProjectCredentialsIfNeeded = (): void => { if (!credentialStore) return; const projectRoot = path.resolve(path.dirname(args.adeDir)); const migratedRoots = readMigratedProjectRoots(); if (migratedRoots.has(projectRoot)) return; const hadEncryptedLegacyStore = fs.existsSync(tokenPath) || fs.existsSync(oauthClientPath); migrateTokenIfMissing(); migrateOAuthClientIfMissing(); if (!hadEncryptedLegacyStore || safeStorage.isEncryptionAvailable()) { markProjectMigrated(); } }; const migrateTokenIfMissing = (): void => { if (readMachineToken()) return; const legacyToken = readEncryptedToken(); if (legacyToken) { persistMachineToken(legacyToken); return; } const legacyPath = path.join(args.adeDir, "local.secret.yaml"); try { const raw = fs.readFileSync(legacyPath, "utf8"); const token = extractLegacyToken(raw); if (token) persistMachineToken({ token, authMode: "manual" }); } catch (error: unknown) { if (!isEnoentError(error)) { args.logger?.warn("linear_sync.token_import_failed", { legacyPath, error: getErrorMessage(error), }); } } }; const migrateOAuthClientIfMissing = (): void => { if (readMachineOAuthClientCredentials()) return; const legacyOAuth = readStoredOAuthClientCredentials() ?? readOAuthConfigFileCredentials(); if (legacyOAuth) persistMachineOAuthClientCredentials(legacyOAuth); };This extraction makes the migration flow clearer: "migrate token, migrate OAuth client, mark as migrated."
🤖 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/cto/linearCredentialService.ts` around lines 270 - 308, The migrateLegacyProjectCredentialsIfNeeded function is doing multiple migration steps in one long block which reduces readability; refactor by extracting the token and OAuth client migration logic into two helper functions (e.g., migrateTokenIfMissing and migrateOAuthClientIfMissing) and call them from migrateLegacyProjectCredentialsIfNeeded while preserving existing guards (credentialStore check), projectRoot/migratedRoots behavior, hadEncryptedLegacyStore computation, the existing error handling/logging (isEnoentError/getErrorMessage), and the final markProjectMigrated call so behavior is unchanged. Ensure migrateTokenIfMissing replicates the readMachineToken/readEncryptedToken/readFileSync/extractLegacyToken/persistMachineToken flow and migrateOAuthClientIfMissing replicates the readMachineOAuthClientCredentials/readStoredOAuthClientCredentials/readOAuthConfigFileCredentials/persistMachineOAuthClientCredentials flow.apps/ade-cli/src/tuiClient/components/FooterControls.tsx (1)
65-274: ⚡ Quick winConsider decomposing FooterControls for clarity.
The component now handles multiple distinct concerns: provider/model display, token tracking, agents availability, scroll state, drawer/pane state, and steer staging. While the current implementation is functional, extracting sub-components (e.g.,
ModeBar,ActionHints) would improve testability and readability.🤖 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/FooterControls.tsx` around lines 65 - 274, FooterControls is doing too many jobs (branding/model display, token/context bar, agents toggle/chip, and action hint logic); refactor by extracting smaller components: create ModeBar (renders brand/modelDisplay/fastMode/reasoningEffort/permissionLabel/pendingSteerCount and uses TokenBar for contextPercent/tokenSummary), ActionHints (encapsulates the actionHint IIFE and uses Hint/TokenBar props like chatScrollOffset/chatScrollMaxOffset/pendingSteerCount/drawerOpen/activePane/drawerMode), and AgentsControls (renders showAgentsToggle, showAgentsChip and related Toggle/Text bits). Replace the corresponding JSX in FooterControls with these new components and pass only the needed props (e.g., provider/modelDisplay/fastMode/reasoningEffort/permissionLabel/contextPercent/tokenSummary/pendingSteerCount to ModeBar; chatScrollOffset/chatScrollMaxOffset/drawerOpen/activePane/drawerMode/pendingSteerCount to ActionHints; agentsAvailable/liveAgentCount/rightPaneShowsAgents/agentsOpen/agentsFocused to AgentsControls) so FooterControls becomes a simple orchestrator.apps/ade-cli/src/tuiClient/components/MentionPalette.tsx (1)
62-64: ⚡ Quick winEnhance textWidth for unicode correctness.
The current implementation uses
[...value].lengthwhich counts Unicode code points, but doesn't account for:
- Combining marks (e.g., é as e + ´)
- Zero-width characters
- Emoji with modifiers or ZWJ sequences
- Full-width vs half-width CJK characters
For terminal rendering, consider using a library like
string-width(handles ANSI and Unicode width) or implementing grapheme cluster counting to ensure accurate column width calculations.🤖 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/MentionPalette.tsx` around lines 62 - 64, The textWidth function currently returns [...value].length which miscalculates terminal column width for combining marks, ZWJ/emoji sequences and full-width CJK characters; replace its implementation in MentionPalette.tsx (function textWidth) to use a proper width calculator such as the string-width package (import stringWidth from 'string-width' and return stringWidth(value)) or, if avoiding dependencies, compute grapheme clusters and map them to display widths (handle ANSI escapes, zero-width and fullwidth characters) so the returned number reflects terminal column width accurately; update any callers/tests that assume the old behavior.apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts (2)
1302-1310: ⚡ Quick winConsider expanding test coverage for
reorderLaneSessionIdsForDisplay.The single test case covers moving a pinned session, but the function likely handles additional scenarios that would benefit from explicit test coverage:
- Moving unpinned sessions
- Using
edge: "before"instead ofedge: "after"- Multiple pinned sessions in the base order
- Edge cases (empty arrays, non-existent session IDs)
Broader coverage would increase confidence in the reordering logic, especially given the complexity of pinned vs unpinned session ordering described in the AI summary.
🤖 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/useWorkSessions.test.ts` around lines 1302 - 1310, Add additional unit tests for reorderLaneSessionIdsForDisplay to cover missing scenarios: create tests for moving an unpinned session (movedSessionId unpinned), using edge: "before" (both pinned and unpinned moves), a baseOrder with multiple pinnedSessionIds, and edge cases such as empty arrays and non-existent session IDs; for each test call reorderLaneSessionIdsForDisplay with appropriate baseOrder, pinnedSessionIds, movedSessionId, targetSessionId and assert the expected array result so the function's pinned vs unpinned ordering behaviour is fully exercised.
352-376: ⚡ Quick winConsider explicitly verifying no refresh during the inactive period.
The test name suggests verification that no blocking refresh occurs while
active: false, but the test only asserts that a refresh happens after re-entering. To make the test's intent clearer and more robust, consider adding an explicit assertion before line 370:listSessionsCachedMock.mockClear(); rerender({ active: false }); await act(async () => { await new Promise((r) => setTimeout(r, 0)); }); // Explicitly verify no refresh during inactive period expect(listSessionsCachedMock).not.toHaveBeenCalled(); rerender({ active: 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/desktop/src/renderer/components/terminals/useWorkSessions.test.ts` around lines 352 - 376, The test for useWorkSessions should explicitly assert that no refresh is triggered while active is false: after calling listSessionsCachedMock.mockClear() and rerender({ active: false }) and awaiting the microtask (the existing act/timeout), add an assertion that listSessionsCachedMock.not.toHaveBeenCalled() before rerender({ active: true }); this ensures the test verifies the inactive-period behavior in addition to the existing post-reentry assertions.apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
1065-1065: ⚡ Quick winConsider clearing
commandMenuAnchorwhencommandMenuTriggeris set tonull.Several code paths set
setCommandMenuTrigger(null)without clearing the anchor state. While this doesn't cause functional issues (since the menu won't render whentriggeris null), it leaves stale positioning data in state. For better state hygiene and to prevent confusion in future maintenance, consider addingsetCommandMenuAnchor(null)alongsidesetCommandMenuTrigger(null)at these locations.Also applies to: 2253-2253, 2408-2408, 2420-2420, 2697-2697, 2708-2708, 2389-2389
🤖 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/chat/AgentChatComposer.tsx` at line 1065, Several code paths call setCommandMenuTrigger(null) but leave stale positioning state in commandMenuAnchor; update each location (e.g., where setCommandMenuTrigger(null) is used around lines noted) to also call setCommandMenuAnchor(null) so both trigger and anchor are cleared together; search for setCommandMenuTrigger(null) and add setCommandMenuAnchor(null) alongside it (ensure you update all instances including the ones referenced) to keep state consistent.apps/ade-cli/src/tuiClient/components/Drawer.tsx (4)
446-453: 💤 Low valueEmpty-chat fallback only triggers when
selectedChatIndex !== 0.When the lane has no sessions and
selectedChatIndex === 0(the very common case where the user just opened the chat block fresh), this branch is skipped and the rendered block shows onlyCHATS · 0followed by+ new chat— no “No chats in lane.” affordance at all. Either drop theselectedChatIndex !== 0guard or document why a selected-state distinguishes the empty-state copy; right now the condition reads like an unintended coupling between selection focus and empty messaging.🤖 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/Drawer.tsx` around lines 446 - 453, The empty-chat fallback currently only renders when sessions.length === 0 && selectedChatIndex !== 0, which hides the “No chats in lane.” message for the common case selectedChatIndex === 0; remove the selectedChatIndex !== 0 guard so the conditional becomes simply sessions.length === 0 (in the Drawer component where the Box/Text fallback is returned) so the “No chats in lane.” affordance always appears for an empty lane.
534-535: ⚡ Quick winDead parameters in
MiniDrawer.
focusedandbrowsingLaneIdare required props onMiniDrawerbut are immediately discarded withvoid. Either drop them from the prop type and the call site (line 173, 168) to keep the contract honest, or actually wire them in (mini drawer currently has no visual "focused" highlight and ignores which lane the user is browsing, which seems inconsistent with the full drawer).🤖 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/Drawer.tsx` around lines 534 - 535, MiniDrawer currently discards the focused and browsingLaneId props (they're voided) creating a broken contract; either remove these props from MiniDrawer's prop type and stop passing them from its callers, or wire them up: remove the two `void` statements, use `focused` to apply the focused/highlighted styling in the MiniDrawer render logic, and use `browsingLaneId` to mark the active lane (e.g., compare lane.id to browsingLaneId and apply active class/aria attributes) so behavior matches the full drawer; update the prop type and call sites accordingly (or delete the props at callers) and ensure TypeScript types and any tests reflect the change.
326-335: 💤 Low value
branchMaxcan exceed the remaining meta width when space is tight.
metaWidthcan collapse to a small or even negative value on very narrow drawers;branchMax = Math.max(3, metaWidth - …)then forces ≥3 chars for the branch even when no horizontal room remains, which can cause the line-2 text to overflow the lane card and visually wrap. Consider clampingbranchMaxtoMath.max(0, metaWidth - …)(or skipping the branch entirely whenmetaWidth <= 0) so the layout degrades gracefully on small terminals.🤖 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/Drawer.tsx` around lines 326 - 335, The branch width calculation allows branchMax to be at least 3 even when metaWidth is small/negative, causing overflow; update the logic around metaWidth/branchMax in Drawer.tsx (the block that computes metaWidth, detailMax, branchMax, truncBranch, truncDetail) to clamp branchMax to not exceed available space (use Math.max(0, metaWidth - (detailMax ? detailMax + 3 : 0)) instead of Math.max(3, ...)) or skip rendering the branch when metaWidth <= 0, and ensure truncate(truncBranch, branchMax) and related rendering handle branchMax === 0 safely to avoid visual wrapping on narrow terminals.
125-125: 💤 Low valueDefault
new Set<string>()allocated every render.Each render of
Drawerallocates a freshSetfor theunavailableLaneIdsdefault, which becomes a new identity for any downstream memoization. Right now nothing here memoizes against it, but if you ever pass it intouseMemo/useCallbackdeps it'll silently invalidate. A module-level frozen empty set is a cheap, safe alternative.🧹 Proposed fix
+const EMPTY_LANE_ID_SET: ReadonlySet<string> = new Set<string>(); + export function Drawer({ ... - unavailableLaneIds = new Set<string>(), + unavailableLaneIds = EMPTY_LANE_ID_SET, }: {🤖 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/Drawer.tsx` at line 125, The default parameter unavailableLaneIds = new Set<string>() in the Drawer component allocates a new Set on every render; replace this with a module-level frozen empty set (e.g. const EMPTY_UNAVAILABLE_LANE_IDS = Object.freeze(new Set<string>())) and use that constant as the default for the unavailableLaneIds parameter/prop in Drawer to preserve identity across renders and avoid accidental memo invalidation.apps/desktop/src/renderer/components/terminals/useWorkSessions.ts (2)
559-572: 💤 Low value
togglePinnedSessionalways replaces state, even when caller-provided id is unchanged.Minor: each call produces a new
pinnedSessionIdsarray (toggle either adds or removes), which is fine for a toggle but means rapid double-calls churn the store. If you ever batch this via an effect or external command, the new array identity will invalidate downstreamuseMemo/useCallbackeven when the logical set is unchanged after two toggles. Not blocking — flagging for awareness.🤖 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/useWorkSessions.ts` around lines 559 - 572, togglePinnedSession always creates a new pinnedSessionIds array even when the resulting set is unchanged; update the callback in togglePinnedSession to check current pinnedSessionIds (from prev.pinnedSessionIds or empty array), compute whether any change is needed (e.g., if sessionId already present and you intend to remove, or absent and intend to add), and if there is no change return prev unchanged; otherwise return the updated state (still adding/removing via pinnedSessionIds) so downstream memoization won't be invalidated by redundant array identity changes. Use the existing symbols togglePinnedSession and setProjectViewState to locate and modify the logic.
521-557: 💤 Low value
reorderLaneSessionsdoesn't surface failures.When the helper returns
null(no movement / invalid args) the callback silently returnsprev. That's fine for the happy path, but the callback also early-returns at line 523 on invalid args without any telemetry/log, and a stalelaneSessionOrder[laneId](e.g. left over from a previously-renamed or removed session id) is left in store untouched even thoughbaseOrderwas rebuilt against the current open set. Consider trimminglaneSessionOrder[laneId]whenever it diverges from the live lane set — otherwise the persisted state grows monotonically as sessions are closed/removed.🤖 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/useWorkSessions.ts` around lines 521 - 557, reorderLaneSessions silently ignores invalid inputs and never trims stale entries in laneSessionOrder; update reorderLaneSessions to (1) emit a telemetry/log when called with invalid args (movedSessionId/targetSessionId missing or equal) instead of silently returning, and (2) when computing baseOrder compare it against prev.laneSessionOrder?.[laneId] and if they diverge replace/trim prev.laneSessionOrder[laneId] with the rebuilt baseOrder (or the filtered existing that matches sessionsInLane) before calling reorderLaneSessionIdsForDisplay so the store doesn't accumulate removed/renamed session ids; use the existing symbols reorderLaneSessions, reorderLaneSessionIdsForDisplay, setProjectViewState, sessionsById and laneSessionOrder to locate where to add logging and the trimming logic.apps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsx (2)
295-314: 💤 Low valueInconsistent newline splitting.
Line 313 uses
older.split("\n")while the rest of the suite (e.g.transcriptLinesat line 46) uses/\r?\n/. If Ink ever emits CRLF on this path the last line will incorrectly contain a trailing\r, which could mask regressions. Align with the helper for consistency.🧹 Proposed fix
- expect(older.split("\n").at(-1)).toContain("↓ newer messages"); + expect(transcriptLines(older).at(-1)).toContain("↓ newer messages");🤖 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/__tests__/ChatView.test.tsx` around lines 295 - 314, The test uses older.split("\n").at(-1) which can leave a trailing '\r' on CRLF outputs; update the assertion to split using the same helper regex as the suite (use older.split(/\r?\n/).at(-1) or reuse the transcriptLines helper) so the last-line check is CRLF-safe; modify the assertion in the "shows the bottom viewport by default and older rows when scrolled" test that references older.split("\n") accordingly.
562-573: 💤 Low value
transcriptBodyflattens viaJSON.stringify— fragile for content assertions.Asserting
transcriptBody.toContain("Subagent started: Explore renderer")againstJSON.stringify(event)works only because the literal string happens to appear inside a stringified field. If any envelope’s event ever serializes that text with embedded escapes (e.g., quotes, newlines, unicode), these assertions can become false positives/negatives. Consider asserting directly on the relevantentry.eventfield (e.g.,description,summary) rather than on a JSON dump.🤖 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/__tests__/ChatView.test.tsx` around lines 562 - 573, The tests build a fragile transcriptBody by JSON.stringify(entry.event) and assert substrings against that; instead, assert directly on the relevant event properties from transcriptEvents (e.g., map transcriptEvents to entry.event.description or entry.event.summary) or find the specific envelope where entry.event.type === "subagent" and assert its description/summary contains "Subagent started: Explore renderer", "found the renderer path", and that no entry.event contains "unrelated agent result"; update the expectations that currently use transcriptBody to use those direct property checks and keep checks for renderEvents/frame (renderEvents, transcriptEvents, entry.event, description, summary).
| if (subagentParentItemIds.size > 0) { | ||
| for (const envelope of args.events) { | ||
| const parentItemId = workEventParentItemId(envelope.event); | ||
| const itemId = workEventItemId(envelope.event); | ||
| if (parentItemId && itemId && subagentParentItemIds.has(parentItemId)) { | ||
| subagentChildItemIds.add(itemId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Filter subagent work transitively, not just one level deep.
subagentChildItemIds is only populated for direct children of a subagent parent item. Any grandchild work item will miss both checks here, so nested command/file-change activity can leak into the main chat timeline instead of staying scoped to the subagent view.
Also applies to: 370-373
🤖 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/aggregate.ts` around lines 287 - 295, The current
loop only adds direct children to subagentChildItemIds, missing nested
descendants; build a transitive closure instead: construct a parent→children map
from args.events (using workEventParentItemId and workEventItemId), then perform
a BFS/DFS starting from subagentParentItemIds to collect all descendant item IDs
into subagentChildItemIds (or alternately iterate adding children until no new
IDs appear). Apply the same fix to the other occurrence where a similar
direct-child-only loop is used.
| if (event.type === "reasoning") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
reasoning events are being dropped instead of aggregated into thinking blocks.
ChatView now has a dedicated thinking renderer, but this branch just skips every reasoning event. That removes thinking output from the transcript entirely and leaves the later turn-completion logic with nothing to finalize.
🤖 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/aggregate.ts` around lines 367 - 369, The branch
that checks if (event.type === "reasoning") should not drop those events;
instead reclassify or merge them into the existing "thinking" aggregation so the
ChatView thinking renderer and the turn-completion logic have content to
finalize. Locate the code handling event processing in aggregate.ts (the block
that contains if (event.type === "reasoning")) and replace the continue with
logic that either converts event.type = "thinking" or appends the reasoning
payload into the current thinking aggregation (e.g., push into the same
buffer/array used for "thinking" events or call the aggregator method that
collects thinking pieces), preserving timestamps/metadata so subsequent
turn-completion/finalize functions can operate normally.
| if (event.type === "done") { | ||
| const startMs = turnId ? turnStart.get(turnId) : undefined; | ||
| const durationMs = startMs !== undefined ? entry.timestamp - startMs : undefined; | ||
| for (const block of blocks) { | ||
| const blockTurn = (block as { turnId?: string | null }).turnId ?? null; | ||
| if (blockTurn !== turnId) continue; | ||
| if (block.kind === "work-block" || block.kind === "thinking" || block.kind === "plan" || block.kind === "compaction") { | ||
| block.live = false; | ||
| if (durationMs !== undefined && (block.kind === "work-block" || block.kind === "thinking")) { | ||
| block.durationMs = block.durationMs ?? durationMs; | ||
| } | ||
| } | ||
| } | ||
| continue; | ||
| } | ||
| if (AGGREGATED_TYPES.has(event.type)) { | ||
| // Already handled above or intentionally skipped (tokens, codex_*). | ||
| continue; |
There was a problem hiding this comment.
turn-footer never gets emitted, so duration/token/cost summaries won't render.
This code finalizes live blocks on done, but it never synthesizes the turn-footer block that ChatView knows how to display. The later aggregated-types skip path also swallows token-usage events, so footer metadata currently has no way to reach the UI.
| function commandAvailable(command: string): boolean { | ||
| const result = spawnSync(process.platform === "win32" ? "where" : "command", process.platform === "win32" ? [command] : ["-v", command], { | ||
| shell: process.platform !== "win32", | ||
| stdio: "ignore", | ||
| }); | ||
| return result.status === 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify commandAvailable logic handles all platforms correctly
# Test the pattern used in commandAvailable
echo "Testing command availability check pattern..."
# Unix-style (should work on macOS/Linux)
if command -v node > /dev/null 2>&1; then
echo "✓ 'command -v' works on $(uname -s)"
else
echo "✗ 'command -v' failed on $(uname -s)"
fi
# Check if the shell parameter is necessary
node --eval "
const { spawnSync } = require('child_process');
// Test without shell
const r1 = spawnSync('command', ['-v', 'node'], { shell: false, stdio: 'ignore' });
console.log('Without shell:', r1.status === 0 ? 'pass' : 'fail');
// Test with shell
const r2 = spawnSync('command', ['-v', 'node'], { shell: true, stdio: 'ignore' });
console.log('With shell:', r2.status === 0 ? 'pass' : 'fail');
"Repository: arul28/ADE
Length of output: 445
Address shell injection vulnerability when passing arguments with shell: true.
The commandAvailable function correctly uses shell: true on Unix platforms where command -v requires shell resolution. However, the Node.js deprecation warning reveals a security vulnerability: arguments passed to spawnSync with shell: true are not escaped and can lead to shell injection attacks if the command parameter contains special characters. Consider refactoring to avoid shell invocation (e.g., using which or type commands without shell spawning) or properly escaping the command argument.
🤖 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/imageTargets.ts` around lines 143 - 149, The
commandAvailable function is vulnerable because it calls spawnSync with shell:
true and unescaped user input; replace the shell invocation with a safe exec of
a non-shell binary on Unix (for example use spawnSync("which", [command],
{stdio:"ignore"}) instead of "command -v" with shell) while keeping Windows' use
of "where" (spawnSync("where", [command], ...)); update commandAvailable to call
the external binary (which/where) without shell:true so arguments are passed as
an argv array and not interpreted by a shell to eliminate injection risk.
| function migrateLegacyKeychainIntoCredentialStore(currentStore: StoredKeys): StoredKeys { | ||
| if (isCredentialLegacyKeychainMigrated()) return currentStore; | ||
| if (!isMacosKeychainAvailable()) return currentStore; | ||
|
|
||
| const index = readMacosKeychainProviderIndex(); | ||
| const providerCandidates = new Set([ | ||
| ...index.providers, | ||
| ...Object.keys(ENV_KEY_PROVIDERS), | ||
| ]); | ||
| const keychainStore = readMacosKeychainStore(providerCandidates); | ||
| const nextStore = mergeLegacyValuesIntoCredentialStore(currentStore, keychainStore); | ||
| markCredentialLegacyKeychainMigrated(); | ||
| return nextStore; |
There was a problem hiding this comment.
Avoid marking keychain migration complete when keychain reads fail
Line 390 currently marks migration complete even when legacy keychain reads may have failed silently (e.g., readMacosKeychainSecret returned null with macosKeychainError). That can permanently disable fallback reads and strand existing keys.
Suggested fix
function migrateLegacyKeychainIntoCredentialStore(currentStore: StoredKeys): StoredKeys {
if (isCredentialLegacyKeychainMigrated()) return currentStore;
if (!isMacosKeychainAvailable()) return currentStore;
+ const errorBefore = macosKeychainError;
const index = readMacosKeychainProviderIndex();
const providerCandidates = new Set([
...index.providers,
...Object.keys(ENV_KEY_PROVIDERS),
]);
const keychainStore = readMacosKeychainStore(providerCandidates);
const nextStore = mergeLegacyValuesIntoCredentialStore(currentStore, keychainStore);
- markCredentialLegacyKeychainMigrated();
+ const hadNewKeychainError = macosKeychainError != null && macosKeychainError !== errorBefore;
+ if (!hadNewKeychainError) {
+ markCredentialLegacyKeychainMigrated();
+ }
return nextStore;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function migrateLegacyKeychainIntoCredentialStore(currentStore: StoredKeys): StoredKeys { | |
| if (isCredentialLegacyKeychainMigrated()) return currentStore; | |
| if (!isMacosKeychainAvailable()) return currentStore; | |
| const index = readMacosKeychainProviderIndex(); | |
| const providerCandidates = new Set([ | |
| ...index.providers, | |
| ...Object.keys(ENV_KEY_PROVIDERS), | |
| ]); | |
| const keychainStore = readMacosKeychainStore(providerCandidates); | |
| const nextStore = mergeLegacyValuesIntoCredentialStore(currentStore, keychainStore); | |
| markCredentialLegacyKeychainMigrated(); | |
| return nextStore; | |
| function migrateLegacyKeychainIntoCredentialStore(currentStore: StoredKeys): StoredKeys { | |
| if (isCredentialLegacyKeychainMigrated()) return currentStore; | |
| if (!isMacosKeychainAvailable()) return currentStore; | |
| const errorBefore = macosKeychainError; | |
| const index = readMacosKeychainProviderIndex(); | |
| const providerCandidates = new Set([ | |
| ...index.providers, | |
| ...Object.keys(ENV_KEY_PROVIDERS), | |
| ]); | |
| const keychainStore = readMacosKeychainStore(providerCandidates); | |
| const nextStore = mergeLegacyValuesIntoCredentialStore(currentStore, keychainStore); | |
| const hadNewKeychainError = macosKeychainError != null && macosKeychainError !== errorBefore; | |
| if (!hadNewKeychainError) { | |
| markCredentialLegacyKeychainMigrated(); | |
| } | |
| return nextStore; | |
| } |
🤖 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/ai/apiKeyStore.ts` around lines 379 - 391, The
migration currently calls markCredentialLegacyKeychainMigrated()
unconditionally, which can disable future fallback reads if macOS keychain reads
failed silently; update migrateLegacyKeychainIntoCredentialStore to only mark
migration as complete when the macOS keychain read succeeded and produced usable
data. Concretely, after calling readMacosKeychainStore (and handling any
underlying readMacosKeychainSecret error indicators), check for success by
verifying there is no macosKeychainError and/or that keychainStore contains
entries (or that mergeLegacyValuesIntoCredentialStore actually changed
currentStore); if the read failed or returned no usable keys, return
currentStore without calling markCredentialLegacyKeychainMigrated(), otherwise
proceed to merge and then call markCredentialLegacyKeychainMigrated(). Ensure
references: migrateLegacyKeychainIntoCredentialStore,
readMacosKeychainProviderIndex, readMacosKeychainStore, readMacosKeychainSecret,
mergeLegacyValuesIntoCredentialStore, markCredentialLegacyKeychainMigrated,
isCredentialLegacyKeychainMigrated, isMacosKeychainAvailable.
| if (roomAbove >= MENU_HEIGHT || roomAbove >= roomBelow) { | ||
| return { | ||
| left, | ||
| width, | ||
| bottom: Math.max(VIEWPORT_GUTTER, viewportHeight - anchor.top + MENU_GAP), | ||
| maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)), | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| left, | ||
| width, | ||
| top: Math.min(viewportHeight - VIEWPORT_GUTTER, anchorBottom + MENU_GAP), | ||
| maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomBelow - MENU_GAP)), | ||
| }; |
There was a problem hiding this comment.
Do not force a 160px minimum maxHeight when viewport space is smaller
The current Math.max(160, ...) can push the menu off-screen in constrained layouts. Use the actual available space (bounded by a small lower floor only if needed for usability, not larger than available room).
Suggested fix
if (roomAbove >= MENU_HEIGHT || roomAbove >= roomBelow) {
return {
left,
width,
bottom: Math.max(VIEWPORT_GUTTER, viewportHeight - anchor.top + MENU_GAP),
- maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)),
+ maxHeight: Math.max(96, Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)),
};
}
return {
left,
width,
top: Math.min(viewportHeight - VIEWPORT_GUTTER, anchorBottom + MENU_GAP),
- maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomBelow - MENU_GAP)),
+ maxHeight: Math.max(96, Math.min(MENU_HEIGHT, roomBelow - MENU_GAP)),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (roomAbove >= MENU_HEIGHT || roomAbove >= roomBelow) { | |
| return { | |
| left, | |
| width, | |
| bottom: Math.max(VIEWPORT_GUTTER, viewportHeight - anchor.top + MENU_GAP), | |
| maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)), | |
| }; | |
| } | |
| return { | |
| left, | |
| width, | |
| top: Math.min(viewportHeight - VIEWPORT_GUTTER, anchorBottom + MENU_GAP), | |
| maxHeight: Math.max(160, Math.min(MENU_HEIGHT, roomBelow - MENU_GAP)), | |
| }; | |
| if (roomAbove >= MENU_HEIGHT || roomAbove >= roomBelow) { | |
| return { | |
| left, | |
| width, | |
| bottom: Math.max(VIEWPORT_GUTTER, viewportHeight - anchor.top + MENU_GAP), | |
| maxHeight: Math.max(96, Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)), | |
| }; | |
| } | |
| return { | |
| left, | |
| width, | |
| top: Math.min(viewportHeight - VIEWPORT_GUTTER, anchorBottom + MENU_GAP), | |
| maxHeight: Math.max(96, Math.min(MENU_HEIGHT, roomBelow - MENU_GAP)), | |
| }; |
🤖 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/chat/ChatCommandMenu.tsx` around lines
92 - 106, The code currently forces a 160px minimum via Math.max(160,
Math.min(MENU_HEIGHT, roomAbove - MENU_GAP)) which can push the menu off-screen;
change both branches to compute available = roomAbove - MENU_GAP (or roomBelow -
MENU_GAP) and set maxHeight to available if available < MIN_FLOOR else
Math.min(MENU_HEIGHT, available) where MIN_FLOOR is a small usability floor
(e.g. 48) — i.e. replace the Math.max(160, Math.min(...)) expressions with a
conditional: if (available < MIN_FLOOR) maxHeight = available else maxHeight =
Math.min(MENU_HEIGHT, available); apply this for the roomAbove and roomBelow
branches (references: MENU_HEIGHT, MENU_GAP, VIEWPORT_GUTTER, roomAbove,
roomBelow, anchor.top/anchorBottom).
| box-shadow: | ||
| inset 0 1px 0 color-mix(in srgb, white 10%, transparent), | ||
| 0 0 0 1px color-mix(in srgb, var(--color-fg) 12%, transparent); | ||
| inset 0 0 0 1px color-mix(in srgb, var(--awaiting-color) 70%, transparent), | ||
| 0 0 0 2px color-mix(in srgb, var(--awaiting-color) 35%, transparent), | ||
| 0 0 14px color-mix(in srgb, var(--awaiting-color) 22%, transparent); |
There was a problem hiding this comment.
Fix stylelint violation in awaiting-state block
Stylelint reports declaration-empty-line-before at Line 2736-2739. Add the required blank line before the declaration to keep CI lint green.
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 2736-2739: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/index.css` around lines 2736 - 2739, The stylelint
error is caused by a missing blank line before the box-shadow declaration in the
awaiting-state block; open the CSS rule that contains the box-shadow property
(the declaration using color-mix with var(--awaiting-color)) and insert a single
empty line immediately before the "box-shadow:" declaration to satisfy
declaration-empty-line-before and restore CI linting.
| shouldCaptureLocalChanges = false | ||
| defer { shouldCaptureLocalChanges = true } |
There was a problem hiding this comment.
Preserve previous capture state instead of forcing it to true
Line 2844 disables capture, but Line 2845 always restores to true. If this method runs under an outer scope that already set shouldCaptureLocalChanges = false, this will re-enable capture too early.
Proposed fix
- shouldCaptureLocalChanges = false
- defer { shouldCaptureLocalChanges = true }
+ let previousCaptureState = shouldCaptureLocalChanges
+ shouldCaptureLocalChanges = false
+ defer { shouldCaptureLocalChanges = previousCaptureState }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shouldCaptureLocalChanges = false | |
| defer { shouldCaptureLocalChanges = true } | |
| let previousCaptureState = shouldCaptureLocalChanges | |
| shouldCaptureLocalChanges = false | |
| defer { shouldCaptureLocalChanges = previousCaptureState } |
🤖 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/ios/ADE/Services/Database.swift` around lines 2844 - 2845, The defer
currently unconditionally sets shouldCaptureLocalChanges = true which can
incorrectly re-enable capturing when the outer scope had it disabled; change
this to preserve and restore the previous value by saving let previous =
shouldCaptureLocalChanges before mutating and then in defer set
shouldCaptureLocalChanges = previous so the method toggles capture idempotently
(refer to the shouldCaptureLocalChanges setter usage in this function).
arul28
force-pushed
the
ade/tui-and-desktop-app-upgrades-1209107e
branch
from
c10870a to
9968f21
Compare
|
@cursor review |
|
@copilot review |
|
@copilot review |
|
@cursor review |
arul28
force-pushed
the
ade/tui-and-desktop-app-upgrades-1209107e
branch
from
0f67cd1 to
e520f7d
Compare
|
@cursor review |
|
@copilot review |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/ade-cli/src/tuiClient/aggregate.ts (3)
284-292:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFilter subagent work transitively, not just one level deep.
subagentChildItemIdsonly captures direct children of subagent parent items. Nested subagent work (grandchildren and deeper) will pass both checks at lines 368-370, so nested command/file-change activity can leak into the main chat timeline instead of staying scoped to the subagent view.🔄 Proposed fix to build transitive closure
const subagentChildItemIds = new Set<string>(); if (subagentParentItemIds.size > 0) { + // Build parent→children map + const childrenMap = new Map<string, Set<string>>(); for (const envelope of args.events) { const parentItemId = workEventParentItemId(envelope.event); const itemId = workEventItemId(envelope.event); - if (parentItemId && itemId && subagentParentItemIds.has(parentItemId)) { - subagentChildItemIds.add(itemId); - } + if (parentItemId && itemId) { + if (!childrenMap.has(parentItemId)) { + childrenMap.set(parentItemId, new Set()); + } + childrenMap.get(parentItemId)!.add(itemId); + } + } + // BFS to collect all transitive descendants + const queue = Array.from(subagentParentItemIds); + while (queue.length > 0) { + const current = queue.shift()!; + const children = childrenMap.get(current); + if (children) { + for (const child of children) { + if (!subagentChildItemIds.has(child)) { + subagentChildItemIds.add(child); + queue.push(child); + } + } + } } }🤖 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/aggregate.ts` around lines 284 - 292, The current loop only adds direct children to subagentChildItemIds; change it to compute the transitive closure so grandchildren and deeper descendants are included: start from the initial subagentParentItemIds (and include any newly discovered child ids) and repeatedly scan args.events using workEventParentItemId(envelope.event) and workEventItemId(envelope.event) to add children into subagentChildItemIds until a full pass adds no new ids; replace the single-pass for-loop with this iterative (while-changed) approach so nested subagent work is fully filtered.
364-366:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reasoningevents are dropped instead of being aggregated.
ChatViewhas a dedicatedthinkingrenderer, but this branch skips everyreasoningevent, removing thinking output from the transcript entirely and leaving turn-completion logic with nothing to finalize.💭 Proposed fix to preserve reasoning events
If reasoning should be treated as thinking blocks, replace the skip with aggregation logic:
if (event.type === "reasoning") { - continue; + // Convert to thinking or aggregate into a thinking block + passthrough(id, "assistant-text"); + continue; }Alternatively, if a dedicated thinking block type exists or should be created, handle it similar to work-block or plan aggregation.
🤖 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/aggregate.ts` around lines 364 - 366, The code currently drops events where event.type === "reasoning" (removing thinking output); instead, preserve and aggregate them by treating "reasoning" like other multi-event blocks: replace the continue with logic that appends/merges the reasoning event into the current aggregation bucket (or invokes the same aggregation/merge routine used for work-block/plan events), so the ChatView thinking renderer still receives the combined thinking content for finalization; locate the branch checking event.type === "reasoning" in aggregate.ts and route those events into the existing aggregation flow rather than skipping them.
488-501:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
turn-footerblock is never emitted, so duration/token/cost summaries won't render.The
turn-footerblock type is defined at line 43 andChatViewpresumably has a renderer for it, but thedoneevent handler only finalizes existing blocks—it never synthesizes aturn-footerblock. Token-usage events are also inAGGREGATED_TYPES(line 184-185) and skipped at line 503-506, so footer metadata has no path to the UI.📊 Proposed fix to emit turn-footer
if (event.type === "done") { const startMs = turnId ? turnStart.get(turnId) : undefined; const durationMs = startMs !== undefined ? entry.timestamp - startMs : undefined; + const tokens = event.usage ? event.usage.inputTokens + event.usage.outputTokens : undefined; + const cost = event.costUsd; for (const block of blocks) { const blockTurn = (block as { turnId?: string | null }).turnId ?? null; if (blockTurn !== turnId) continue; if (block.kind === "work-block" || block.kind === "plan" || block.kind === "compaction") { block.live = false; if (durationMs !== undefined && block.kind === "work-block") { block.durationMs = block.durationMs ?? durationMs; } } } + if (turnId) { + blocks.push({ + kind: "turn-footer", + id, + turnId, + durationMs, + tokens, + cost, + }); + } continue; }🤖 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/aggregate.ts` around lines 488 - 501, When handling event.type === "done" you finalize existing blocks but never emit the synthesized "turn-footer" block, so duration/token/cost summaries never reach the UI; update the done-event branch to create and push a new block of kind "turn-footer" (including computed durationMs from turnStart.get(turnId), aggregated token/cost data from AGGREGATED_TYPES events or your aggregation state for that turn, and any metadata) only when turnId is present, then append it to blocks before continuing; ensure you still skip raw AGGREGATED_TYPES events later but use the aggregated values to populate the new "turn-footer" so ChatView can render summaries.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/prs/prService.ts (1)
6087-6088: ⚡ Quick winScope snapshot hydration to the selected PR IDs
listSnapshotRows()loads/deserializes snapshots for the whole project on every call, but this method only needs checks for the lane display rows you just selected. Fetching only those PR IDs will reduce avoidable DB/JSON overhead on a likely frequently-called path.♻️ Proposed change
- const checksByPrId = new Map(listSnapshotRows().map((snapshot) => [snapshot.prId, snapshot.checks] as const)); + const checksByPrId = new Map<string, PrCheck[]>(); + for (const row of rows) { + const snapshot = listSnapshotRows({ prId: row.id })[0]; + checksByPrId.set(row.id, snapshot?.checks ?? []); + }🤖 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/prs/prService.ts` around lines 6087 - 6088, The code currently calls listSnapshotRows() for the whole project then builds checksByPrId for every snapshot; instead scope snapshot hydration to only the PR IDs present in rows: collect the selectedPrIds from rows (e.g., rows.map(r => r.id)), call a new or existing API that accepts an array of PR IDs (e.g., listSnapshotRowsForPrIds(selectedPrIds) or add a parameter to listSnapshotRows(prIds)), build checksByPrId from that restricted snapshot result, and pass checksByPrId.get(row.id) to rowToLanePrSummary (keeping isActivePrState logic unchanged); update or add the targeted snapshot-loading function to deserialize only those PR IDs to avoid full-project deserialization.apps/ade-cli/src/services/sync/syncHostService.ts (1)
3300-3317: 💤 Low valueReuse the new
stopBonjourAnnouncement/stopNativeLanDiscoveryhelpers indispose.Now that
unpublishLanDiscovery()already invokesstopNativeLanDiscovery()andstopBonjourAnnouncement(), thebonjourAnnouncement.stop?.()block below is redundant (it is alwaysnullby the time control reaches it). Folding thebonjourInstance.destroy()call into the existing teardown helpers keeps lifecycle logic in one place.♻️ Proposed cleanup
unpublishLanDiscovery(); try { await unpublishTailnetDiscovery(); } catch { // Never throw from dispose. } await new Promise<void>((resolve) => { // ... }); - if (bonjourAnnouncement) { - try { - bonjourAnnouncement.stop?.(); - } catch { - // ignore cleanup failures - } - bonjourAnnouncement = null; - } - bonjourPort = null; - bonjourSignature = null; if (bonjourInstance) { try { bonjourInstance.destroy(); } catch { // ignore cleanup failures } bonjourInstance = null; }🤖 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/services/sync/syncHostService.ts` around lines 3300 - 3317, The dispose logic currently calls bonjourAnnouncement.stop and bonjourInstance.destroy directly even though unpublishLanDiscovery already calls stopNativeLanDiscovery() and stopBonjourAnnouncement(); remove the redundant bonjourAnnouncement.stop?.() and the bonjourInstance.destroy() call from dispose and instead invoke the existing helpers stopBonjourAnnouncement() and stopNativeLanDiscovery() (or call unpublishLanDiscovery() if that consolidates both) so teardown is centralized; ensure bonjourPort and bonjourSignature are still nulled after using the helpers and that any try/catch behavior from the helpers is preserved.
🤖 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/sync/syncHostService.ts`:
- Around line 1299-1310: The exit/error handlers for the native dns-sd child
(handlers on child.once("error") and child.once("exit")) currently only clear
nativeBonjourProcess and log, so they never re-spawn or fall back; update these
handlers to attempt auto-recovery by calling publishLanDiscovery(port, { force:
true }) (or trigger the Bonjour fallback) when the active child fails or exits,
but gate the recovery with a short debounce/throttle to avoid rapid respawns if
dns-sd is permanently broken; ensure you check nativeBonjourProcess === child
before scheduling recovery and clear any scheduled recovery when a new
nativeBonjourProcess is created to prevent fork-bombing.
In `@apps/ade-cli/src/tuiClient/workEventIds.ts`:
- Line 11: The function workEventParentItemId currently casts the event to {
parentItemId?: unknown } which hides that parentItemId exists only on
"tool_call" and "tool_result"; update workEventParentItemId(AgentChatEvent) to
explicitly narrow the event by checking event.type === "tool_call" || event.type
=== "tool_result" and return null for other types, then call
textField(event.parentItemId) without a cast; keep textField usage and apply the
same explicit type-narrowing pattern to any similar helper if needed.
In `@apps/desktop/src/renderer/components/usage/UsageQuotaPanel.tsx`:
- Around line 320-326: The current code treats "weekly" and "monthly" as
interchangeable by finding a single weeklyWindow that matches either type;
instead, split them into separate variables (weeklyWindow and monthlyWindow),
keep fiveHourWindow as is, and adjust otherWindows to exclude all three
(weeklyWindow, monthlyWindow, fiveHourWindow); where a single window is needed
for the sparkline use the weekly-first fallback weeklyWindow ?? monthlyWindow to
match selectPacingWindow and MissionHeader.tsx behavior and preserve priority
and labels.
---
Duplicate comments:
In `@apps/ade-cli/src/tuiClient/aggregate.ts`:
- Around line 284-292: The current loop only adds direct children to
subagentChildItemIds; change it to compute the transitive closure so
grandchildren and deeper descendants are included: start from the initial
subagentParentItemIds (and include any newly discovered child ids) and
repeatedly scan args.events using workEventParentItemId(envelope.event) and
workEventItemId(envelope.event) to add children into subagentChildItemIds until
a full pass adds no new ids; replace the single-pass for-loop with this
iterative (while-changed) approach so nested subagent work is fully filtered.
- Around line 364-366: The code currently drops events where event.type ===
"reasoning" (removing thinking output); instead, preserve and aggregate them by
treating "reasoning" like other multi-event blocks: replace the continue with
logic that appends/merges the reasoning event into the current aggregation
bucket (or invokes the same aggregation/merge routine used for work-block/plan
events), so the ChatView thinking renderer still receives the combined thinking
content for finalization; locate the branch checking event.type === "reasoning"
in aggregate.ts and route those events into the existing aggregation flow rather
than skipping them.
- Around line 488-501: When handling event.type === "done" you finalize existing
blocks but never emit the synthesized "turn-footer" block, so
duration/token/cost summaries never reach the UI; update the done-event branch
to create and push a new block of kind "turn-footer" (including computed
durationMs from turnStart.get(turnId), aggregated token/cost data from
AGGREGATED_TYPES events or your aggregation state for that turn, and any
metadata) only when turnId is present, then append it to blocks before
continuing; ensure you still skip raw AGGREGATED_TYPES events later but use the
aggregated values to populate the new "turn-footer" so ChatView can render
summaries.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 3300-3317: The dispose logic currently calls
bonjourAnnouncement.stop and bonjourInstance.destroy directly even though
unpublishLanDiscovery already calls stopNativeLanDiscovery() and
stopBonjourAnnouncement(); remove the redundant bonjourAnnouncement.stop?.() and
the bonjourInstance.destroy() call from dispose and instead invoke the existing
helpers stopBonjourAnnouncement() and stopNativeLanDiscovery() (or call
unpublishLanDiscovery() if that consolidates both) so teardown is centralized;
ensure bonjourPort and bonjourSignature are still nulled after using the helpers
and that any try/catch behavior from the helpers is preserved.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 6087-6088: The code currently calls listSnapshotRows() for the
whole project then builds checksByPrId for every snapshot; instead scope
snapshot hydration to only the PR IDs present in rows: collect the selectedPrIds
from rows (e.g., rows.map(r => r.id)), call a new or existing API that accepts
an array of PR IDs (e.g., listSnapshotRowsForPrIds(selectedPrIds) or add a
parameter to listSnapshotRows(prIds)), build checksByPrId from that restricted
snapshot result, and pass checksByPrId.get(row.id) to rowToLanePrSummary
(keeping isActivePrState logic unchanged); update or add the targeted
snapshot-loading function to deserialize only those PR IDs to avoid full-project
deserialization.
🪄 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: e69079da-0699-4d8d-8a29-af2469bb0d08
⛔ Files ignored due to path filters (7)
apps/desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/features/ade-code/README.mdis excluded by!docs/**docs/features/cto/README.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (137)
.claude/skills/plan/SKILL.md.github/workflows/release-core.ymlapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsxapps/ade-cli/src/tuiClient/__tests__/HeaderFooter.test.tsxapps/ade-cli/src/tuiClient/__tests__/Palettes.test.tsxapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/__tests__/drawerSelection.test.tsapps/ade-cli/src/tuiClient/__tests__/eventDedup.test.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/__tests__/heartbeat.test.tsapps/ade-cli/src/tuiClient/__tests__/imageTargets.test.tsapps/ade-cli/src/tuiClient/__tests__/jsonRpcClient.test.tsapps/ade-cli/src/tuiClient/__tests__/keybindings.test.tsapps/ade-cli/src/tuiClient/__tests__/linearCommands.test.tsapps/ade-cli/src/tuiClient/__tests__/project.test.tsapps/ade-cli/src/tuiClient/__tests__/subagentPane.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/FooterControls.tsxapps/ade-cli/src/tuiClient/components/Header.tsxapps/ade-cli/src/tuiClient/components/MentionPalette.tsxapps/ade-cli/src/tuiClient/components/ModelStatus.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/components/SlashPalette.tsxapps/ade-cli/src/tuiClient/drawerSelection.tsapps/ade-cli/src/tuiClient/format.tsapps/ade-cli/src/tuiClient/imageTargets.tsapps/ade-cli/src/tuiClient/keybindings/index.tsapps/ade-cli/src/tuiClient/laneTree.tsapps/ade-cli/src/tuiClient/project.tsapps/ade-cli/src/tuiClient/spinTick.tsxapps/ade-cli/src/tuiClient/state.tsapps/ade-cli/src/tuiClient/subagentPane.tsapps/ade-cli/src/tuiClient/theme.tsapps/ade-cli/src/tuiClient/types.tsapps/ade-cli/src/tuiClient/workEventIds.tsapps/desktop/package.jsonapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/apiKeyStore.test.tsapps/desktop/src/main/services/ai/apiKeyStore.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/claudeOutputStyles.test.tsapps/desktop/src/main/services/chat/claudeOutputStyles.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.test.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.tsapps/desktop/src/main/services/cto/issueTracker.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearClient.tsapps/desktop/src/main/services/cto/linearCredentialService.tsapps/desktop/src/main/services/cto/linearDispatcherService.tsapps/desktop/src/main/services/cto/linearIssueTracker.tsapps/desktop/src/main/services/cto/linearLaneCardService.test.tsapps/desktop/src/main/services/cto/linearLaneCardService.tsapps/desktop/src/main/services/cto/linearSync.test.tsapps/desktop/src/main/services/diffs/diffService.test.tsapps/desktop/src/main/services/diffs/diffService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/opencode/openCodeRuntime.test.tsapps/desktop/src/main/services/opencode/openCodeRuntime.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/automations/adeActionSchemas.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/ChatCommandMenu.tsxapps/desktop/src/renderer/components/lanes/laneDesignTokens.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/renderer/components/terminals/LaneChip.tsxapps/desktop/src/renderer/components/terminals/ProviderChip.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/renderer/components/usage/UsageQuotaPanel.test.tsxapps/desktop/src/renderer/components/usage/UsageQuotaPanel.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/git.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/usage.tsapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsPinSheet.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (4)
- apps/ade-cli/src/tuiClient/tests/heartbeat.test.ts
- apps/ade-cli/src/tuiClient/tests/jsonRpcClient.test.ts
- apps/ade-cli/src/tuiClient/tests/linearCommands.test.ts
- apps/ade-cli/src/tuiClient/tests/eventDedup.test.ts
✅ Files skipped from review due to trivial changes (7)
- apps/desktop/src/shared/types/git.ts
- apps/desktop/package.json
- apps/desktop/src/main/services/cto/linearIssueTracker.ts
- apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts
- .claude/skills/plan/SKILL.md
- apps/desktop/src/main/services/cto/linearLaneCardService.test.ts
- apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift
🚧 Files skipped from review as they are similar to previous changes (110)
- apps/ade-cli/src/tuiClient/drawerSelection.ts
- apps/desktop/src/main/utils/terminalSessionSignals.test.ts
- apps/ade-cli/src/tuiClient/tests/drawerSelection.test.ts
- apps/ade-cli/src/tuiClient/tests/keybindings.test.ts
- apps/ade-cli/src/tuiClient/tests/appInput.test.ts
- apps/ade-cli/src/tuiClient/keybindings/index.ts
- apps/desktop/src/renderer/components/lanes/laneDesignTokens.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx
- apps/desktop/src/main/services/ai/aiIntegrationService.test.ts
- apps/ade-cli/src/tuiClient/state.ts
- apps/desktop/src/shared/types/prs.ts
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/renderer/components/terminals/LaneChip.tsx
- apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/main/services/cto/linearDispatcherService.ts
- apps/desktop/src/main/services/cto/issueTracker.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/renderer/components/terminals/SessionCard.tsx
- apps/desktop/src/main/services/lanes/laneListSnapshotService.test.ts
- apps/desktop/src/main/services/adeActions/registry.test.ts
- apps/desktop/src/main/services/cto/linearSync.test.ts
- apps/desktop/src/renderer/components/automations/adeActionSchemas.ts
- apps/desktop/src/renderer/components/terminals/ProviderChip.tsx
- apps/desktop/src/shared/cliLaunch.ts
- apps/desktop/src/main/services/lanes/laneListSnapshotService.ts
- apps/ade-cli/src/services/sync/syncService.ts
- apps/desktop/src/shared/types/chat.ts
- apps/ade-cli/src/tuiClient/tests/commands.test.ts
- apps/desktop/src/renderer/components/usage/UsageQuotaPanel.test.tsx
- apps/ade-cli/src/tuiClient/tests/Palettes.test.tsx
- apps/desktop/src/main/services/sync/syncHostService.test.ts
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
- apps/desktop/src/main/services/diffs/diffService.test.ts
- apps/desktop/src/renderer/state/appStore.test.ts
- apps/ade-cli/src/tuiClient/tests/imageTargets.test.ts
- apps/desktop/src/shared/types/usage.ts
- apps/ade-cli/src/tuiClient/laneTree.ts
- apps/desktop/src/main/utils/terminalSessionSignals.ts
- apps/ade-cli/src/tuiClient/components/Header.tsx
- apps/ade-cli/src/tuiClient/tests/HeaderFooter.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx
- apps/ade-cli/src/headlessLinearServices.ts
- apps/ade-cli/src/tuiClient/components/ModelStatus.tsx
- apps/ade-cli/src/tuiClient/tests/adeApi.test.ts
- apps/ade-cli/src/tuiClient/tests/subagentPane.test.ts
- apps/ios/ADE/Views/Settings/SettingsPinSheet.swift
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/renderer/components/app/TopBar.test.tsx
- apps/desktop/src/main/services/cto/linearAuth.test.ts
- apps/desktop/src/main/services/cto/linearLaneCardService.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
- apps/ade-cli/src/tuiClient/components/MentionPalette.tsx
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/sync/syncService.test.ts
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
- apps/desktop/src/main/services/cto/linearCredentialService.ts
- apps/desktop/src/renderer/components/chat/ChatCommandMenu.tsx
- apps/ade-cli/src/tuiClient/tests/Drawer.test.tsx
- apps/ade-cli/src/tuiClient/adeApi.ts
- apps/desktop/src/renderer/components/app/AppShell.tsx
- apps/desktop/src/preload/preload.ts
- apps/ade-cli/src/tuiClient/project.ts
- apps/desktop/src/main/services/pty/ptyService.test.ts
- apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts
- apps/ade-cli/src/tuiClient/tests/connection.test.ts
- apps/ade-cli/src/tuiClient/types.ts
- apps/ade-cli/src/tuiClient/tests/project.test.ts
- apps/ade-cli/src/cli.ts
- apps/desktop/src/renderer/components/usage/HeaderUsageControl.tsx
- apps/ade-cli/src/tuiClient/components/FooterControls.tsx
- apps/ade-cli/src/tuiClient/tests/format.test.ts
- apps/ade-cli/src/tuiClient/commands.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts
- apps/ade-cli/src/tuiClient/tests/RightPane.test.tsx
- apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts
- apps/ade-cli/src/tuiClient/theme.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/ai/apiKeyStore.test.ts
- apps/ios/ADE/Services/Database.swift
- apps/desktop/src/renderer/components/app/TopBar.tsx
- .github/workflows/release-core.yml
- apps/desktop/src/main/services/usage/usageTrackingService.test.ts
- apps/desktop/src/renderer/index.css
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/components/terminals/WorkSidebar.tsx
- apps/desktop/src/main/services/sessions/sessionService.ts
- apps/desktop/src/main/services/diffs/diffService.ts
- apps/ade-cli/src/tuiClient/subagentPane.ts
- apps/ade-cli/src/tuiClient/format.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/ade-cli/src/tuiClient/components/Drawer.tsx
- apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx
- apps/desktop/src/main/services/pty/ptyService.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/main/services/ai/apiKeyStore.ts
- apps/desktop/src/main/services/usage/usageTrackingService.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/ade-cli/src/tuiClient/components/RightPane.tsx
- apps/desktop/src/main/services/chat/claudeOutputStyles.test.ts
- apps/ade-cli/src/tuiClient/imageTargets.ts
- apps/ade-cli/src/tuiClient/components/SlashPalette.tsx
- apps/desktop/src/main/services/chat/agentChatService.ts
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/ade-cli/src/tuiClient/components/ChatView.tsx
- apps/ios/ADE/Services/SyncService.swift
|
@copilot review |
|
@cursor review |
|
@cursor review |
|
@copilot review |
| "codex_goal_updated", | ||
| "codex_goal_cleared", | ||
| "pending_input_resolved", | ||
| ]); |
There was a problem hiding this comment.
AGGREGATED_TYPES missing system_notice and activity types
Low Severity
The AGGREGATED_TYPES set is used as a guard at line 510 to skip events already handled above. But system_notice, activity, context_compact, codex_context_compaction, and status are all handled by explicit if branches earlier in the loop yet are not listed in AGGREGATED_TYPES. When these event types reach the bottom fallthrough, the AGGREGATED_TYPES.has(event.type) check misses them. For system_notice events that aren't steer lifecycle or memory notices, and for context_compact/codex_context_compaction/status/activity, this doesn't cause a problem because they're handled before reaching line 510. But if new system_notice subtypes are added that don't match any existing branch, they'll fall through to the generic "notice" block instead of being silently skipped like other aggregated types.
Reviewed by Cursor Bugbot for commit c274574. Configure here.
|
@cursor review |
|
@copilot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f11519d. Configure here.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Note
Medium Risk
Medium risk: introduces new TUI rendering/aggregation and LAN discovery code paths plus stricter release-asset validation, which could affect runtime connectivity and release pipelines if edge cases were missed.
Overview
Delivers a major TUI UX upgrade: chat rendering now aggregates events into higher-level blocks (work/tool groups, plans, compaction, memory, queued steers) and hides low-signal noise, with new helpers for scrolling, transcript copy/export, subagent transcript viewing, and richer palettes/header/footer behavior.
Adds new runtime-backed surfaces for lane context (bulk diff line stats, per-lane PR summaries, and “worktree missing” handling), plus steer-message lifecycle support via new
chatdomain actions (steer/edit/cancel/dispatch).Improves sync discovery reliability by adding a native
dns-sdpublisher on macOS Electron with fallback/retry behavior and explicit LAN refresh forcing, and tightens release automation by validating the publish asset manifest (including runtime tar contents) before drafting GitHub releases. Also adds a new/planskill spec document for three-round locked planning.Reviewed by Cursor Bugbot for commit f11519d. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR delivers a large-scale TUI and desktop app upgrade: a new
aggregateChatBlocksrendering model, staged steer chat controls (steer/editSteer/cancelSteer/dispatchSteer), new API endpoints for lane diff stats and per-lane PR summaries, native macOS Electron LAN discovery viadns-sdwith fallback/recovery logic, usage threshold event emission, and a release asset validation step in CI.aggregate.ts): introducesAggregatedBlockunion type andaggregateChatBlocksto group tools/edits/plan/memory/compaction/queued-steers into high-level blocks, with broad test coverage.syncHostService.ts): on macOS Electron, spawnsdns-sd -Rinstead of Bonjour; on failure sets a 30-second Bonjour fallback, then retries native after the window expires.release-core.yml): adds a pre-publish manifest check that validates expected DMG, ZIP, blockmap, YAML, runtime binary, and native tarball artifacts before drafting the GitHub release.Confidence Score: 5/5
Safe to merge; no blocking defects found. All new paths have tests and graceful error handling.
The LAN discovery fallback/recovery logic is carefully guarded (disposed flag, port checks, 30-second Bonjour window), the aggregated block rendering is well-tested, and the new API endpoints follow existing patterns. The desktop test mock for node:child_process is narrower than the CLI counterpart but does not cause current failures. The linkedAt value passed to the Linear card callback is technically redundant rather than wrong. CI artifact validation is a net improvement even with the minor diagnostic gap in the tar check.
.github/workflows/release-core.yml tar validation and apps/desktop/src/main/services/sync/syncHostService.test.ts mock completeness are worth a second look.
Important Files Changed
Sequence Diagram
sequenceDiagram participant SHS as SyncHostService participant dnsd as dns-sd (native) participant Bonjour as Bonjour (fallback) SHS->>SHS: publishLanDiscovery(port) SHS->>SHS: shouldUseNativeLanDiscovery()? alt macOS + Electron + fallback window expired SHS->>dnsd: spawn("dns-sd", ["-R", name, ...txt]) dnsd-->>SHS: child.unref() note over SHS,dnsd: Running indefinitely dnsd--xSHS: exit/error event SHS->>SHS: "nativeLanDiscoveryFallbackUntilMs = now+30s" SHS->>SHS: scheduleNativeLanDiscoveryRecovery(port) [1s timer] SHS->>SHS: [1s later] publishLanDiscovery(port) note over SHS: shouldUseNativeLanDiscovery() = false (within 30s) SHS->>Bonjour: bonjourInstance.publish(...) else Linux/Windows or fallback active SHS->>Bonjour: bonjourInstance.publish(...) end SHS->>SHS: dispose() SHS->>dnsd: child.kill("SIGTERM") SHS->>Bonjour: bonjourAnnouncement.stop()Comments Outside Diff (2)
apps/desktop/src/main/services/ai/apiKeyStore.ts, line 84-88 (link)__setSafeStorageForTestsmissesmissingCredentialProvidersresetThis PR adds
missingCredentialProvidersas module-level state (line 82), andinitApiKeyStorecorrectly clears it on re-init. However,__setSafeStorageForTests— used by tests that swap thesafeStoragemock between cases — does not resetmissingCredentialProviders. If a test that exercises the credential-store path fillsmissingCredentialProvidersand a subsequent test calls only__setSafeStorageForTests, the stale "missing" entries will causegetApiKeyto skip thecredentialStoreread and returnnullfor those providers.Prompt To Fix With AI
apps/desktop/src/main/services/usage/usageTrackingService.ts, line 629-631 (link)When both
messageIdandrequestIdare empty strings,dedupeKeyis":". The guardif (dedupeKey !== ":" && seen.has(dedupeKey)) continuebypasses deduplication entirely for these entries, potentially inflating the 30-day cost estimate. Compare withscanClaudeLogswhich unconditionally deduplicates.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "ship: iteration 5 - settle cursor follow..." | Re-trigger Greptile