Conversation
Contributor
📝 WalkthroughWalkthroughAdds ACP-aware remote control (per-channel default agents and workdir resolution), dual-track remote block streaming (temporary status + streamed answer) for Telegram/Feishu with delivery-state persistence and message mutation, finalization fixes in DeepChat presenter, renderer UI/workdir settings and i18n, plus extensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router as Remote Router
participant Runner as RemoteConversationRunner
participant Config as ConfigPresenter
participant Presenter as SessionCreator
User->>Router: /new (channel)
Router->>Runner: createNewSession(agentId, endpointKey)
Runner->>Config: getAgentType(agentId)
Config-->>Runner: 'acp' or other
alt agent is ACP
Runner->>Runner: resolveDefaultWorkdir(endpointKey)
alt workdir found
Runner->>Presenter: createDetachedSession({providerId:'acp', modelId:agentId, projectDir:workdir})
Presenter-->>Runner: session created
Runner-->>Router: success (session)
Router-->>User: reply (workdir shown)
else no workdir
Runner-->>Router: throw actionable error
Router-->>User: error message
end
else non-ACP agent
Runner->>Presenter: createDetachedSession({modelId:agentId})
Presenter-->>Runner: session created
Runner-->>Router: success
Router-->>User: reply
end
sequenceDiagram
participant Poller as Telegram/Feishu Poller
participant RunnerSnapshot as Runner Snapshot
participant Binding as RemoteBindingStore
participant Client as Platform Client
loop poll
Poller->>RunnerSnapshot: fetch snapshot (statusText, text, finalText)
RunnerSnapshot-->>Poller: snapshot
Poller->>Binding: getRemoteDeliveryState(endpointKey)
Binding-->>Poller: state|null
alt no state (first stream)
Poller->>Client: sendMessage(statusText)
Client-->>Poller: statusMessageId
Poller->>Client: sendMessage(chunk1)
Client-->>Poller: contentId1
Poller->>Binding: rememberRemoteDeliveryState(...)
else have state (incremental)
Poller->>Client: editMessage(statusMessageId, newStatus)
Poller->>Client: editMessage(contentIdN, newTail)
Poller->>Binding: rememberRemoteDeliveryState(updated)
end
alt completed
Poller->>Client: editMessage(contentIdN, finalText)
Poller->>Client: deleteMessage(statusMessageId)
Poller->>Binding: clearRemoteDeliveryState(endpointKey)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts (1)
227-243:⚠️ Potential issue | 🟡 MinorNormalize blank default workdirs before formatting
/status.
getDefaultWorkdir()can still surface the empty-string state used by the new setting, anddefaultWorkdir ?? 'none'will render that as a blank field instead ofnone. Normalize it withtrim() || 'none', likeCurrent workdir.💡 Suggested fix
- const defaultWorkdir = await this.deps.runner.getDefaultWorkdir(endpointKey) + const defaultWorkdir = (await this.deps.runner.getDefaultWorkdir(endpointKey))?.trim() @@ - `Default workdir: ${defaultWorkdir ?? 'none'}`, + `Default workdir: ${defaultWorkdir || 'none'}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts` around lines 227 - 243, The `status` reply can render an empty-string default workdir as a blank field; update the `Default workdir` formatting in remoteCommandRouter's status case to normalize blank values by trimming and treating empty/whitespace as missing (use the result of getDefaultWorkdir(), call .trim() safely and fall back to 'none'), similar to how `Current workdir` is handled; locate the `Default workdir: ${defaultWorkdir ?? 'none'}` expression and change it to use a trimmed-or-'none' fallback.
🧹 Nitpick comments (8)
src/main/presenter/remoteControlPresenter/index.ts (1)
723-737: Keep the fallback default-agent ID canonicalized.The first two branches return alias-resolved IDs, but the final fallback writes
enabledAgents[0]?.idraw. ApplyingresolveAcpAgentAlias()there too keeps persisted defaults stable for legacy ACP aliases.♻️ Suggested cleanup
const nextDefaultAgentId = enabledAgentIds.has(normalizedCandidate) ? normalizedCandidate : enabledAgentIds.has(TELEGRAM_REMOTE_DEFAULT_AGENT_ID) ? TELEGRAM_REMOTE_DEFAULT_AGENT_ID - : enabledAgents[0]?.id || TELEGRAM_REMOTE_DEFAULT_AGENT_ID + : resolveAcpAgentAlias( + enabledAgents[0]?.id || TELEGRAM_REMOTE_DEFAULT_AGENT_ID + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/index.ts` around lines 723 - 737, In sanitizeDefaultAgentId the final fallback returns enabledAgents[0]?.id without canonicalizing, causing non-canonical IDs to be persisted; change that branch to return resolveAcpAgentAlias(enabledAgents[0]?.id || TELEGRAM_REMOTE_DEFAULT_AGENT_ID) so the fallback is passed through resolveAcpAgentAlias (keeping the earlier normalization behavior used for normalizedCandidate and TELEGRAM_REMOTE_DEFAULT_AGENT_ID).test/main/presenter/remoteControlPresenter/remoteControlPresenter.test.ts (1)
390-427: Consider mirroring this regression coverage for Feishu.
saveFeishuSettings()picked up the samedefaultWorkdirpersistence insrc/main/presenter/remoteControlPresenter/index.ts, but the new assertions here only exercise Telegram. One Feishu save/get case would protect the second channel too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/remoteControlPresenter.test.ts` around lines 390 - 427, Add a parallel test to cover Feishu like the Telegram case: inside the same suite create a RemoteControlPresenter (same setup using createConfigPresenter and createHooksConfig) and call presenter.saveFeishuSettings(...) with matching fields (remoteEnabled: true, defaultAgentId: 'acp-agent', defaultWorkdir: '/workspaces/acp', hookNotifications similar to the Telegram test), then assert the returned object (from saveFeishuSettings) preserves defaultAgentId and defaultWorkdir; reference RemoteControlPresenter and the saveFeishuSettings method so the regression that affected Feishu’s defaultWorkdir persistence is exercised.test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts (1)
184-234: Add a/statuscase for the empty-workdir fallback.The new assertions only cover a populated
getDefaultWorkdir()result, so the''/blank path behind “leave empty” would still regress silently. A second case assertingDefault workdir: nonewould lock that behavior down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts` around lines 184 - 234, Add a second test case in remoteCommandRouter.test.ts that covers the empty-workdir fallback by instantiating RemoteCommandRouter with createRunner where getDefaultWorkdir is mocked to resolve to '' and getPollerStatus remains as in the existing test, then call router.handleMessage(createMessage({ text: '/status', command: { name: 'status', args: '' } })) and assert the reply contains "Default workdir: none" (in addition to any other relevant assertions); locate the test setup using RemoteCommandRouter, createRunner, getDefaultWorkdir, handleMessage and createMessage to add this new spec mirroring the existing '/status' test but with getDefaultWorkdir returning ''.src/renderer/settings/components/RemoteSettings.vue (1)
222-300: Consider extracting the workdir dropdown to a reusable component.The Telegram default workdir dropdown (lines 222-300) is nearly identical to the Feishu version (lines 598-676). This duplication increases maintenance burden - any bug fix or enhancement needs to be applied twice.
♻️ Suggested extraction
Create a
DefaultWorkdirDropdown.vuecomponent:<script setup lang="ts"> defineProps<{ modelValue: string label: string title: string options: Array<{ path: string; name: string }> testIdPrefix: string }>() defineEmits<{ select: [path: string] pick: [] clear: [] }>() </script> <template> <div class="space-y-2"> <Label class="text-xs text-muted-foreground">{{ label }}</Label> <DropdownMenu> <!-- ... shared dropdown content ... --> </DropdownMenu> </div> </template>Then use it in both Telegram and Feishu sections:
<DefaultWorkdirDropdown :model-value="telegramSettings.defaultWorkdir" :label="t('settings.remote.remoteControl.defaultWorkdir')" :title="telegramDefaultWorkdirTitle" :options="telegramDirectoryOptions" test-id-prefix="remote" `@select`="(path) => selectDefaultWorkdir('telegram', path)" `@pick`="pickDefaultWorkdir('telegram')" `@clear`="clearDefaultWorkdir('telegram')" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/RemoteSettings.vue` around lines 222 - 300, The Telegram default workdir dropdown is duplicated (nearly identical to Feishu); extract it into a reusable component (e.g., DefaultWorkdirDropdown.vue) that accepts props modelValue (bind to telegramSettings.defaultWorkdir), label, title, options (use telegramDirectoryOptions), and a testIdPrefix, and emits select, pick, and clear; then replace the inline dropdown in RemoteSettings.vue with <DefaultWorkdirDropdown> and forward handlers to the existing methods selectDefaultWorkdir('telegram', path), pickDefaultWorkdir('telegram'), and clearDefaultWorkdir('telegram') so behavior and tests remain unchanged.src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts (1)
336-347: Redundant delivery state initialization.The delivery state is initialized here when
sourceMessageIdis truthy, butprepareDeliveryStateForSource(lines 412-420) already returns a new delivery state object whensourceMessageIdis provided andstateis null. This creates duplicate initialization logic.Consider simplifying by relying solely on
prepareDeliveryStateForSource:♻️ Suggested simplification
if (sourceMessageId) { - deliveryState = deliveryState ?? { - sourceMessageId, - statusMessageId: null, - contentMessageIds: [], - lastStatusText: '', - lastContentText: '' - } - deliveryState = await this.syncStatusMessage(target, endpointKey, deliveryState, statusText) deliveryState = await this.syncContentText(target, endpointKey, deliveryState, streamText) }Since
prepareDeliveryStateForSourcealready guarantees a non-null state whensourceMessageIdis truthy, the null coalescing here is defensive but redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts` around lines 336 - 347, The initialization of deliveryState inside the sourceMessageId branch is redundant because prepareDeliveryStateForSource already returns a non-null delivery state when sourceMessageId is provided; remove the defensive coalescing block that sets deliveryState = deliveryState ?? { sourceMessageId, statusMessageId: null, contentMessageIds: [], lastStatusText: '', lastContentText: '' } and instead call prepareDeliveryStateForSource to obtain the state before invoking syncStatusMessage and syncContentText (keep using deliveryState, sourceMessageId, syncStatusMessage, syncContentText and prepareDeliveryStateForSource to locate the relevant code).docs/specs/telegram-remote-control/plan.md (1)
88-98: Minor: Consider varying sentence structure in testing section.Static analysis flagged three consecutive sentences starting with "Unit tests". While this is a minor style issue, varying the phrasing improves readability.
📝 Suggested rewording
## Testing Strategy - Unit tests for `remoteAuthGuard`. - Unit tests for `remoteBindingStore`. -- Unit tests for `remoteCommandRouter`. -- Unit tests for `remoteConversationRunner`. +- Router tests for `remoteCommandRouter`. +- Runner tests for `remoteConversationRunner`. - Unit tests for `telegramParser`. -- Unit tests for `telegramClient` request payloads. +- Client tests for `telegramClient` request payloads. - Unit tests for `telegramOutbound` chunking/final-text behavior. -- Unit tests for Telegram command registration, callback handling, and message reaction lifecycle behavior. -- Unit tests for temporary-status deletion, streamed-answer updates, pending interaction prompting, and long-answer continuation behavior. +- Integration tests for Telegram command registration, callback handling, and message reaction lifecycle behavior. +- Delivery tests for temporary-status deletion, streamed-answer updates, pending interaction prompting, and long-answer continuation behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/telegram-remote-control/plan.md` around lines 88 - 98, The list under "Testing Strategy" repeats "Unit tests for ..." three times and uses the same sentence structure throughout; vary phrasing to improve readability by rewording some items to use synonyms and different sentence starts (e.g., "Write tests that verify...", "Add unit tests covering...", "Verify via unit tests that...", or "Include tests for...") while keeping the same coverage for remoteAuthGuard, remoteBindingStore, remoteCommandRouter, remoteConversationRunner, telegramParser, telegramClient, telegramOutbound and the items about Telegram command registration/callbacks, temporary-status deletion, streamed-answer updates, pending interaction prompting, and long-answer continuation.docs/specs/remote-acp-control/plan.md (1)
64-64: Minor: Leading space in code span is intentional but flagged by linter.The
(ACP)suffix has a leading space which is intentional for proper display (e.g., "My Agent (ACP)"). The markdownlint warning about spaces inside code spans can be safely ignored here, or you could rephrase to avoid the code span:ACP agents display with an "(ACP)" suffix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-acp-control/plan.md` at line 64, The code span "` (ACP)`" in the sentence "ACP agents display with a ` (ACP)` suffix." contains an intentional leading space that triggers markdownlint; update the text to avoid the lint warning by replacing the code span with a plain string or adjusted span—e.g., change to 'ACP agents display with an "(ACP)" suffix.' or '`(ACP)`' (no leading space) so the display intent remains but the linter no longer flags it.src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
544-553: Clarify: Only the last chunk is editable during streaming.The logic
editableIndex = Math.max(0, contentMessageIds.length - 1)means only the final chunk can be updated during streaming. Earlier chunks remain frozen. This is likely intentional (Feishu API constraint?), but the implicit behavior could confuse maintainers.Consider adding a brief comment explaining why only the last chunk is editable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts` around lines 544 - 553, The loop that computes editableIndex and updates message chunks (editableIndex = Math.max(0, contentMessageIds.length - 1); for ... if (previousChunks[index] === nextChunks[index]) ... this.deps.client.updateText(contentMessageIds[index], nextChunks[index])) intentionally only allows the last chunk to be updated during streaming; add a short clarifying comment above the editableIndex calculation explaining why earlier chunks are immutable (e.g., Feishu API only permits editing the most recently posted segment or to avoid UI flicker/race conditions), referencing variables editableIndex, contentMessageIds, previousChunks, nextChunks and the updateText call so future maintainers understand the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/remote-acp-control/tasks.md`:
- Line 50: The markdown code span contains a leading space (` (ACP)`) causing
MD038; update the list item "- [x] Label ACP agents with ` (ACP)`" so the code
span has no leading space (i.e., use backticks around "(ACP)" -> ` (ACP)` →
`(ACP)` inside the backticks), ensuring the text becomes "- [x] Label ACP agents
with `(ACP)`".
In `@docs/specs/remote-multi-channel/spec.md`:
- Line 7: Update the phrasing that currently reads "one streamed answer message"
to clarify it represents a single streamed answer track or sequence rather than
a single physical message—replace occurrences of the exact phrase "one streamed
answer message" (and the related sentence in Lines 28-30 that permits
multi-chunk delivery) with wording such as "one streamed answer track (one
logical message sequence, delivered in one or more physical chunks as needed)"
so the document consistently allows multi-chunk overflow while keeping the
logical single-answer semantics.
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts`:
- Around line 246-248: The finalizeTrailingPendingNarrativeBlocks call (and
subsequent state.blocks.push of stagedResult.searchPayload.block) is currently
branch-local and misses the plain tool-result path; move the finalization out of
these branches into the shared tool-execution completion boundary in dispatch.ts
so it always runs once before processing completed tool calls (e.g. before
handling tool_call_end results), then make the append/push logic branch-free;
update the analogous spots referenced (the other
finalizeTrailingPendingNarrativeBlocks uses around the 387-388 and 441-442
occurrences) and add a dispatch-level test that sends tool_call_end followed by
trailing text to ensure pending narrative blocks are finalized immediately.
In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts`:
- Around line 183-191: The status rendering uses `defaultWorkdir ?? 'none'`,
which still shows blank when `defaultWorkdir` is an empty string or whitespace;
update the rendering to normalize the value by trimming and falling back to
'none' (e.g., compute a normalizedWorkdir = (defaultWorkdir ?? '').trim() ||
'none') before including it in the replies; change the `Default workdir: ${...}`
interpolation in the replies block (where `defaultWorkdir` is retrieved via
`this.deps.runner.getDefaultWorkdir(endpointKey)`) to use the normalized value.
In `@src/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.ts`:
- Around line 370-463: The renderer buildRemoteRenderableBlocks currently
handles only content, reasoning_content, tool_call, search, image, and error but
silently skips AssistantMessageBlock types plan, action, audio, and
artifact-thinking; add a clear inline comment inside buildRemoteRenderableBlocks
(near the top of the for-loop or before the cascade of ifs) documenting that
those four types are intentionally unhandled in this v1 remote renderer (explain
constraints: private-chat only, no media upload) and explicitly call out that
action blocks are processed separately via collectPendingInteraction so
reviewers know they are not lost; keep the comment concise and reference the
exact unhandled type names and the collectPendingInteraction handler.
---
Outside diff comments:
In `@src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts`:
- Around line 227-243: The `status` reply can render an empty-string default
workdir as a blank field; update the `Default workdir` formatting in
remoteCommandRouter's status case to normalize blank values by trimming and
treating empty/whitespace as missing (use the result of getDefaultWorkdir(),
call .trim() safely and fall back to 'none'), similar to how `Current workdir`
is handled; locate the `Default workdir: ${defaultWorkdir ?? 'none'}` expression
and change it to use a trimmed-or-'none' fallback.
---
Nitpick comments:
In `@docs/specs/remote-acp-control/plan.md`:
- Line 64: The code span "` (ACP)`" in the sentence "ACP agents display with a `
(ACP)` suffix." contains an intentional leading space that triggers
markdownlint; update the text to avoid the lint warning by replacing the code
span with a plain string or adjusted span—e.g., change to 'ACP agents display
with an "(ACP)" suffix.' or '`(ACP)`' (no leading space) so the display intent
remains but the linter no longer flags it.
In `@docs/specs/telegram-remote-control/plan.md`:
- Around line 88-98: The list under "Testing Strategy" repeats "Unit tests for
..." three times and uses the same sentence structure throughout; vary phrasing
to improve readability by rewording some items to use synonyms and different
sentence starts (e.g., "Write tests that verify...", "Add unit tests
covering...", "Verify via unit tests that...", or "Include tests for...") while
keeping the same coverage for remoteAuthGuard, remoteBindingStore,
remoteCommandRouter, remoteConversationRunner, telegramParser, telegramClient,
telegramOutbound and the items about Telegram command registration/callbacks,
temporary-status deletion, streamed-answer updates, pending interaction
prompting, and long-answer continuation.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 544-553: The loop that computes editableIndex and updates message
chunks (editableIndex = Math.max(0, contentMessageIds.length - 1); for ... if
(previousChunks[index] === nextChunks[index]) ...
this.deps.client.updateText(contentMessageIds[index], nextChunks[index]))
intentionally only allows the last chunk to be updated during streaming; add a
short clarifying comment above the editableIndex calculation explaining why
earlier chunks are immutable (e.g., Feishu API only permits editing the most
recently posted segment or to avoid UI flicker/race conditions), referencing
variables editableIndex, contentMessageIds, previousChunks, nextChunks and the
updateText call so future maintainers understand the constraint.
In `@src/main/presenter/remoteControlPresenter/index.ts`:
- Around line 723-737: In sanitizeDefaultAgentId the final fallback returns
enabledAgents[0]?.id without canonicalizing, causing non-canonical IDs to be
persisted; change that branch to return
resolveAcpAgentAlias(enabledAgents[0]?.id || TELEGRAM_REMOTE_DEFAULT_AGENT_ID)
so the fallback is passed through resolveAcpAgentAlias (keeping the earlier
normalization behavior used for normalizedCandidate and
TELEGRAM_REMOTE_DEFAULT_AGENT_ID).
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 336-347: The initialization of deliveryState inside the
sourceMessageId branch is redundant because prepareDeliveryStateForSource
already returns a non-null delivery state when sourceMessageId is provided;
remove the defensive coalescing block that sets deliveryState = deliveryState ??
{ sourceMessageId, statusMessageId: null, contentMessageIds: [], lastStatusText:
'', lastContentText: '' } and instead call prepareDeliveryStateForSource to
obtain the state before invoking syncStatusMessage and syncContentText (keep
using deliveryState, sourceMessageId, syncStatusMessage, syncContentText and
prepareDeliveryStateForSource to locate the relevant code).
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Around line 222-300: The Telegram default workdir dropdown is duplicated
(nearly identical to Feishu); extract it into a reusable component (e.g.,
DefaultWorkdirDropdown.vue) that accepts props modelValue (bind to
telegramSettings.defaultWorkdir), label, title, options (use
telegramDirectoryOptions), and a testIdPrefix, and emits select, pick, and
clear; then replace the inline dropdown in RemoteSettings.vue with
<DefaultWorkdirDropdown> and forward handlers to the existing methods
selectDefaultWorkdir('telegram', path), pickDefaultWorkdir('telegram'), and
clearDefaultWorkdir('telegram') so behavior and tests remain unchanged.
In `@test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts`:
- Around line 184-234: Add a second test case in remoteCommandRouter.test.ts
that covers the empty-workdir fallback by instantiating RemoteCommandRouter with
createRunner where getDefaultWorkdir is mocked to resolve to '' and
getPollerStatus remains as in the existing test, then call
router.handleMessage(createMessage({ text: '/status', command: { name: 'status',
args: '' } })) and assert the reply contains "Default workdir: none" (in
addition to any other relevant assertions); locate the test setup using
RemoteCommandRouter, createRunner, getDefaultWorkdir, handleMessage and
createMessage to add this new spec mirroring the existing '/status' test but
with getDefaultWorkdir returning ''.
In `@test/main/presenter/remoteControlPresenter/remoteControlPresenter.test.ts`:
- Around line 390-427: Add a parallel test to cover Feishu like the Telegram
case: inside the same suite create a RemoteControlPresenter (same setup using
createConfigPresenter and createHooksConfig) and call
presenter.saveFeishuSettings(...) with matching fields (remoteEnabled: true,
defaultAgentId: 'acp-agent', defaultWorkdir: '/workspaces/acp',
hookNotifications similar to the Telegram test), then assert the returned object
(from saveFeishuSettings) preserves defaultAgentId and defaultWorkdir; reference
RemoteControlPresenter and the saveFeishuSettings method so the regression that
affected Feishu’s defaultWorkdir persistence is exercised.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 246ebfdf-c3c3-490b-87e5-163f04ac6e69
📒 Files selected for processing (50)
docs/specs/remote-acp-control/plan.mddocs/specs/remote-acp-control/spec.mddocs/specs/remote-acp-control/tasks.mddocs/specs/remote-block-streaming/plan.mddocs/specs/remote-block-streaming/spec.mddocs/specs/remote-block-streaming/tasks.mddocs/specs/remote-multi-channel/plan.mddocs/specs/remote-multi-channel/spec.mddocs/specs/telegram-remote-control/plan.mddocs/specs/telegram-remote-control/spec.mdsrc/main/presenter/deepchatAgentPresenter/accumulator.tssrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/telegram/telegramClient.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/remoteControlPresenter/types.tssrc/renderer/settings/components/RemoteSettings.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/presenters/remote-control.presenter.d.tstest/main/presenter/deepchatAgentPresenter/accumulator.test.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteBindingStore.test.tstest/main/presenter/remoteControlPresenter/remoteBlockRenderer.test.tstest/main/presenter/remoteControlPresenter/remoteCommandRouter.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.tstest/main/presenter/remoteControlPresenter/telegramClient.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/RemoteSettings.test.ts
| ## T4 Settings UI And i18n | ||
|
|
||
| - [x] Show all enabled agents in the default-agent selector | ||
| - [x] Label ACP agents with ` (ACP)` |
Contributor
There was a problem hiding this comment.
Fix markdown lint warning: space inside code span.
The backticks around (ACP) include a leading space, which triggers a markdown lint warning (MD038).
📝 Suggested fix
-- [x] Label ACP agents with ` (ACP)`
+- [x] Label ACP agents with `(ACP)` suffix📝 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.
Suggested change
| - [x] Label ACP agents with ` (ACP)` | |
| - [x] Label ACP agents with `(ACP)` suffix |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 50-50: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/remote-acp-control/tasks.md` at line 50, The markdown code span
contains a leading space (` (ACP)`) causing MD038; update the list item "- [x]
Label ACP agents with ` (ACP)`" so the code span has no leading space (i.e., use
backticks around "(ACP)" -> ` (ACP)` → `(ACP)` inside the backticks), ensuring
the text becomes "- [x] Label ACP agents with `(ACP)`".
src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts
Outdated
Show resolved
Hide resolved
src/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts`:
- Line 521: finalizePendingNarrativeBeforeToolExecution(state) only updates
state.blocks in memory; ensure you persist that update before invoking
long-running tools by calling the existing persistence routine
(persistToolExecutionState or the function used elsewhere to save state)
immediately after finalizePendingNarrativeBeforeToolExecution(state) and before
awaiting toolPresenter.callTool(...). In short: flush the finalized narrative to
storage right after finalizePendingNarrativeBeforeToolExecution and only then
await toolPresenter.callTool so consumers won't see the old pending narrative
while the tool runs.
In `@src/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.ts`:
- Around line 287-307: The switch on latestBlock.type (used after
getLastMeaningfulBlock) can fall through and return undefined for unhandled
types like plan, audio, and artifact-thinking; add a default branch to the
switch that returns a safe string (e.g., DEFAULT_REMOTE_STATUS_TEXT) to satisfy
the declared string return, and optionally add explicit cases for 'plan',
'audio', and 'artifact-thinking' returning appropriate status messages (similar
style to 'image' or 'content'); update the switch in remoteBlockRenderer.ts so
every possible latestBlock.type path returns a string.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f88716ba-556f-4234-b7e6-2ee2ae4b3f24
📒 Files selected for processing (6)
docs/specs/remote-acp-control/tasks.mddocs/specs/remote-multi-channel/spec.mdsrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/main/presenter/deepchatAgentPresenter/dispatch.test.ts
- docs/specs/remote-acp-control/tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts
| pendingInteractions: PendingToolInteraction[] | ||
| terminalError?: string | ||
| }> { | ||
| finalizePendingNarrativeBeforeToolExecution(state) |
Contributor
There was a problem hiding this comment.
Flush the finalized narrative before awaiting tool calls.
This only updates state.blocks in memory. On the normal tool path, the first
persistToolExecutionState(...) still happens after await toolPresenter.callTool(...), so
long-running tools leave consumers seeing the old pending narrative state until the tool
returns.
Possible fix
finalizePendingNarrativeBeforeToolExecution(state)
for (const tc of state.completedToolCalls) {
const toolDef = tools.find((t) => t.function.name === tc.name)
if (!toolDef) continue
const block = state.blocks.find((b) => b.type === 'tool_call' && b.tool_call?.id === tc.id)
if (!block?.tool_call) continue
block.tool_call.server_name = toolDef.server.name
block.tool_call.server_icons = toolDef.server.icons
block.tool_call.server_description = toolDef.server.description
}
+
+ persistToolExecutionState(io, state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts` at line 521,
finalizePendingNarrativeBeforeToolExecution(state) only updates state.blocks in
memory; ensure you persist that update before invoking long-running tools by
calling the existing persistence routine (persistToolExecutionState or the
function used elsewhere to save state) immediately after
finalizePendingNarrativeBeforeToolExecution(state) and before awaiting
toolPresenter.callTool(...). In short: flush the finalized narrative to storage
right after finalizePendingNarrativeBeforeToolExecution and only then await
toolPresenter.callTool so consumers won't see the old pending narrative while
the tool runs.
Comment on lines
+287
to
+307
| switch (latestBlock.type) { | ||
| case 'reasoning_content': | ||
| return 'Running: thinking...' | ||
| case 'content': | ||
| return 'Running: writing...' | ||
| case 'tool_call': { | ||
| const toolName = normalizeText(latestBlock.tool_call?.name) || 'tool' | ||
| return latestBlock.status === 'pending' || latestBlock.status === 'loading' | ||
| ? `Running: calling ${toolName}...` | ||
| : 'Running: processing tool results...' | ||
| } | ||
| case 'search': | ||
| return 'Running: reviewing search results...' | ||
| case 'image': | ||
| return 'Running: preparing image output...' | ||
| case 'error': | ||
| return DEFAULT_REMOTE_STATUS_TEXT | ||
| case 'action': | ||
| return REMOTE_WAITING_STATUS_TEXT | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add default case to prevent implicit undefined return.
The switch statement handles most block types but omits plan, audio, and artifact-thinking. Since getLastMeaningfulBlock (line 268) can return any block type that isn't an action with needsUserAction, if the last meaningful block is one of these unhandled types, the function falls through without a return statement, implicitly returning undefined instead of the declared string type.
🐛 Proposed fix
case 'error':
return DEFAULT_REMOTE_STATUS_TEXT
case 'action':
return REMOTE_WAITING_STATUS_TEXT
+ default:
+ return DEFAULT_REMOTE_STATUS_TEXT
}
}📝 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.
Suggested change
| switch (latestBlock.type) { | |
| case 'reasoning_content': | |
| return 'Running: thinking...' | |
| case 'content': | |
| return 'Running: writing...' | |
| case 'tool_call': { | |
| const toolName = normalizeText(latestBlock.tool_call?.name) || 'tool' | |
| return latestBlock.status === 'pending' || latestBlock.status === 'loading' | |
| ? `Running: calling ${toolName}...` | |
| : 'Running: processing tool results...' | |
| } | |
| case 'search': | |
| return 'Running: reviewing search results...' | |
| case 'image': | |
| return 'Running: preparing image output...' | |
| case 'error': | |
| return DEFAULT_REMOTE_STATUS_TEXT | |
| case 'action': | |
| return REMOTE_WAITING_STATUS_TEXT | |
| } | |
| } | |
| switch (latestBlock.type) { | |
| case 'reasoning_content': | |
| return 'Running: thinking...' | |
| case 'content': | |
| return 'Running: writing...' | |
| case 'tool_call': { | |
| const toolName = normalizeText(latestBlock.tool_call?.name) || 'tool' | |
| return latestBlock.status === 'pending' || latestBlock.status === 'loading' | |
| ? `Running: calling ${toolName}...` | |
| : 'Running: processing tool results...' | |
| } | |
| case 'search': | |
| return 'Running: reviewing search results...' | |
| case 'image': | |
| return 'Running: preparing image output...' | |
| case 'error': | |
| return DEFAULT_REMOTE_STATUS_TEXT | |
| case 'action': | |
| return REMOTE_WAITING_STATUS_TEXT | |
| default: | |
| return DEFAULT_REMOTE_STATUS_TEXT | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.ts`
around lines 287 - 307, The switch on latestBlock.type (used after
getLastMeaningfulBlock) can fall through and return undefined for unhandled
types like plan, audio, and artifact-thinking; add a default branch to the
switch that returns a safe string (e.g., DEFAULT_REMOTE_STATUS_TEXT) to satisfy
the declared string return, and optionally add explicit cases for 'plan',
'audio', and 'artifact-thinking' returning appropriate status messages (similar
style to 'image' or 'content'); update the switch in remoteBlockRenderer.ts so
every possible latestBlock.type path returns a string.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/presenter/toolPresenter/agentTools/agentToolManager.ts (2)
534-535: Centralize question-tool behavioral description to prevent divergenceThe MCP description duplicates policy text maintained elsewhere. Consider exporting one shared description constant and using it in both schema/presenter surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts` around lines 534 - 535, Extract the duplicated MCP description string into a single exported constant (e.g., AGENT_PAUSE_TOOL_DESCRIPTION) and import/use that constant wherever the same text is currently duplicated (including the description field in AgentToolManager's pause/question tool and the corresponding schema/presenter surfaces) so both places reference the same source of truth; update symbol references in agentToolManager.ts (the description property) and the schema/presenter modules to use the new exported constant and remove the inline duplicate strings.
359-361: ReuseQUESTION_TOOL_CONTRACT_HINTinstead of duplicating contract textThis message now duplicates contract wording from
questionTool.ts; drift will eventually create inconsistent guidance.♻️ Proposed refactor
-import { questionToolSchema, QUESTION_TOOL_NAME } from '@/lib/agentRuntime/questionTool' +import { + questionToolSchema, + QUESTION_TOOL_NAME, + QUESTION_TOOL_CONTRACT_HINT +} from '@/lib/agentRuntime/questionTool' ... - throw new Error( - `Invalid arguments for ${QUESTION_TOOL_NAME}. Use a single object with \`header?\`, \`question\`, \`options\`, \`multiple?\`, and \`custom?\`. Ask exactly one question per tool call. Do not use \`questions\` or \`allowOther\`, and do not pass stringified \`options\` JSON. Validation details: ${validationResult.error.message}` - ) + throw new Error( + `Invalid arguments for ${QUESTION_TOOL_NAME}. ${QUESTION_TOOL_CONTRACT_HINT} Validation details: ${validationResult.error.message}` + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts` around lines 359 - 361, Replace the duplicated contract text in the error thrown for QUESTION_TOOL_NAME with the shared constant QUESTION_TOOL_CONTRACT_HINT: update the throw in agentToolManager (the block that currently builds the Error with `Invalid arguments for ${QUESTION_TOOL_NAME}... Validation details: ${validationResult.error.message}`) to concatenate a short prefix/context with QUESTION_TOOL_CONTRACT_HINT and the validationResult.error.message so the message reuses the canonical contract text rather than embedding a duplicate string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/toolPresenter/agentTools/agentToolManager.ts`:
- Around line 534-535: Extract the duplicated MCP description string into a
single exported constant (e.g., AGENT_PAUSE_TOOL_DESCRIPTION) and import/use
that constant wherever the same text is currently duplicated (including the
description field in AgentToolManager's pause/question tool and the
corresponding schema/presenter surfaces) so both places reference the same
source of truth; update symbol references in agentToolManager.ts (the
description property) and the schema/presenter modules to use the new exported
constant and remove the inline duplicate strings.
- Around line 359-361: Replace the duplicated contract text in the error thrown
for QUESTION_TOOL_NAME with the shared constant QUESTION_TOOL_CONTRACT_HINT:
update the throw in agentToolManager (the block that currently builds the Error
with `Invalid arguments for ${QUESTION_TOOL_NAME}... Validation details:
${validationResult.error.message}`) to concatenate a short prefix/context with
QUESTION_TOOL_CONTRACT_HINT and the validationResult.error.message so the
message reuses the canonical contract text rather than embedding a duplicate
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d70b29f-f5ee-4a2c-ba19-c5caee1bd516
📒 Files selected for processing (5)
docs/specs/question-tool-prompt-optimization/spec.mdsrc/main/lib/agentRuntime/questionTool.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/index.tstest/main/presenter/toolPresenter/toolPresenter.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/specs/question-tool-prompt-optimization/spec.md
- src/main/presenter/toolPresenter/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI
Commands / UX
/statusshows Default and Current workdir; ACP-backed sessions lock model selection.Docs / Tests