Fix remote media delivery and ACP workdir handling#1535
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens remote answer streaming rules; adds inbound attachment download/decryption and outbound generated-image persistence/delivery; extends adapters/clients with image send/download APIs; adds runner.sendInput to accept attachments and persist files; enforces per-channel defaultWorkdir with ACP validation and replaces text inputs with dropdown pickers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Remote User
participant Adapter as Channel Adapter
participant Client as Channel Client
participant Runner as RemoteConversationRunner
participant Agent as Agent/Session
participant Workspace as Workspace Storage
User->>Adapter: send message with attachments
Adapter->>Client: parse/normalize attachments
Client-->>Adapter: attachments metadata
Adapter->>Runner: sendInput(text, attachments, sourceMessageId)
Note over Runner: Prepare attachment files
loop each attachment
alt data present
Runner->>Workspace: write file from data
else encrypted media
Runner->>Client: download CDN with encrypt params
Client-->>Runner: encrypted bytes
Runner->>Runner: decrypt AES-128-ECB, write file
else remote URL
Runner->>Client: download file
Client-->>Runner: base64 data
Runner->>Workspace: write file
end
end
Runner->>Agent: sendMessage(text, { files })
Agent-->>Runner: snapshot (may include generatedImages)
Note over Runner: persist generated images
loop each generated image
Runner->>Workspace: save image asset
end
Runner->>Adapter: sendMessage(finalText) %% may be empty if images present
loop each generated image
Runner->>Adapter: sendImage(imagePath)
alt success
Adapter->>Client: sendImage -> delivered
else failure
Runner->>Adapter: sendMessage(fallback text with path)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.ts (1)
528-561:⚠️ Potential issue | 🟡 MinorMake
appendTerminalDeliverySegmentprivate or remove it—it's unused in the runtime flow.The method is never called within weixinIlinkRuntime.ts itself. The completion branch (lines 419–447) syncs only process segments, then sends
finalTextseparately. The non-completion branch (lines 449–459) syncs only process segments as well. Since it lacks aprivatemodifier, it's accidentally public. Other runtimes (telegram, feishu, discord) declare this methodprivateand call it internally; here it appears to be dead code from a prior refactor. Either mark itprivateand remove the test call, or delete it entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.ts` around lines 528 - 561, The method appendTerminalDeliverySegment in weixinIlinkRuntime.ts is unused and exposed publicly; remove the dead code by deleting the entire appendTerminalDeliverySegment implementation (or, if you intend to keep it for internal reuse, change its declaration to private and ensure any test-only calls are removed), matching other runtimes that declare this helper as private (e.g., telegram/feishu/discord); update any imports/usages if present to avoid dangling references.src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts (1)
140-207:⚠️ Potential issue | 🟡 MinorValidate
file_infobefore sending media messages in both C2C and group image methods.The
QQBotFileUploadResponsetype hasfile_infoas an optional property. If the upload response omitsfile_info, the subsequent message POST sendsmedia: { file_info: undefined }(serialized as{}). QQBot will reject with an opaque error unrelated to the upload failure. Add validation immediately after each upload to fail fast with a clear error message:const media = await this.uploadC2CFile(target.openId, target.filePath) if (!media.file_info) { throw new Error('QQBot image upload did not return file_info.') }Apply the same check in
sendGroupImageafteruploadGroupFile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts` around lines 140 - 207, The upload response may omit file_info (QQBotFileUploadResponse has it optional), so in sendC2CImage and sendGroupImage, after calling uploadC2CFile(target.openId, target.filePath) and uploadGroupFile(target.groupOpenId, target.filePath) respectively, validate that media.file_info exists and throw a clear error (e.g., throw new Error('QQBot image upload did not return file_info.')) to fail fast instead of posting media with undefined file_info; reference sendC2CImage, sendGroupImage, uploadC2CFile, uploadGroupFile, and the file_info field when adding this check.
♻️ Duplicate comments (1)
src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts (1)
253-288:⚠️ Potential issue | 🟡 MinorDownload failure mirrors Feishu: silent for the user.
Same concern as flagged in
feishuRuntime.ts—ondownloadFileBase64failure the attachment is preserved withoutdataand the user gets no indication the file couldn't be ingested. Resolving this consistently across channels would be nice.🤖 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 253 - 288, The resolveMessageAttachments method currently leaves attachments without data silent on downloadFileBase64 failures; update the catch block to annotate the attachment with a download error and surface that to the returned TelegramInboundMessage so callers can detect failures. Specifically, when downloadFileBase64 (used in TelegramPoller.resolveMessageAttachments) throws, return the original attachment augmented with a machine-readable error field (e.g., downloadError: string) and also set a message-level indicator on the returned TelegramInboundMessage (e.g., failedAttachments: array of filenames or a boolean attachmentsFailed) so downstream code can notify the user or handle retries; keep the existing console.warn but include the error.message text in the downloadError value.
🧹 Nitpick comments (17)
src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts (1)
190-198:filenamein the return type is declared but never populated.Minor: the return type advertises an optional
filename, but the implementation never sets it, and the caller infeishuRuntime.tsdoesn't read it either. Drop the field from the type (or populate it from the SDK response /params.fileKey) so the contract isn't misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts` around lines 190 - 198, The declared optional filename in downloadMessageResource's Promise return is never set; either remove filename from the return type or populate it (e.g., extract from the SDK response metadata or derive from params.fileKey) and return it alongside data and mediaType; update any caller such as feishuRuntime.ts if it needs to consume the filename (or remove its expectations) so the function signature and actual returned object stay consistent.src/main/presenter/remoteControlPresenter/discord/discordClient.ts (1)
141-142: Prefer a static import andpath.basenamefor readability.The dynamic
import('node:fs/promises')is inconsistent with the static import pattern used in sibling clients (e.g.,weixinIlinkClient.ts), and splittingfilePathwith a regex duplicates whatnode:path.basenamealready does and won't handle edge cases like trailing separators.♻️ Suggested change
+import { readFile } from 'node:fs/promises' +import { basename } from 'node:path' ... - const fileBuffer = await import('node:fs/promises').then((fs) => fs.readFile(filePath)) - const fileName = filePath.split(/[\\/]/).pop() || 'image' + const fileBuffer = await readFile(filePath) + const fileName = basename(filePath) || 'image'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/discord/discordClient.ts` around lines 141 - 142, Replace the dynamic import and manual path splitting: statically import fs/promises (used by fileBuffer assignment) at the top of the module and use path.basename from 'path' to compute fileName (replace the fileName = filePath.split... line) so you avoid dynamic import overhead and handle edge cases like trailing separators; update references to fileBuffer and fileName accordingly (look for the fileBuffer assignment and fileName variable in discordClient.ts).test/main/presenter/remoteControlPresenter/feishuParser.test.ts (1)
90-127: Consider also covering thefilebranch.The parser has a symmetric
message_type === 'file'branch with afile_namefallback (file_name.trim() || file_key) andmediaType: 'application/octet-stream'. Adding an analogous test would guard against regressions in the file path and the fallback-filename logic, which isn't exercised elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts` around lines 90 - 127, Add a new unit test covering the parser's file branch: call FeishuParser.parseEvent with message.message_type === 'file' and content JSON including file_key and file_name cases (both non-empty and whitespace-only) to verify attachments use filename fallback logic (file_name.trim() || file_key) and that mediaType is 'application/octet-stream'; assert parsed.text is '' and attachments contain id/resourceKey equal to file_key and filename set to either trimmed file_name or file_key when file_name is blank.src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts (1)
211-226: Attachment-vs-text dispatch is duplicated across all four channel routers.The exact same
attachments.length > 0 ? sendInput(...) : sendText(...)shape now lives infeishuCommandRouter.ts,qqbotCommandRouter.ts,discordCommandRouter.ts,weixinIlinkCommandRouter.ts, andremoteCommandRouter.ts, only differing bysourceMessageIdcoercion andbindingMeta. Consider collapsing this into a single helper (e.g.runner.sendFromInboundMessage({ endpointKey, text, attachments, sourceMessageId, bindingMeta })) to keep future changes (new fields, validation, logging) from having to be cross-ported manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts` around lines 211 - 226, Duplicate conditional dispatch of attachments vs text across routers should be consolidated into a single helper on the runner (e.g., add a method like sendFromInboundMessage) that accepts {endpointKey, text, attachments, sourceMessageId, bindingMeta}, performs the attachments.length > 0 check and calls the existing sendInput or sendText accordingly (preserving sourceMessageId coercion logic currently used in feishuCommandRouter.ts), and returns the same response shape; update feishuCommandRouter.ts (and the other routers) to call deps.runner.sendFromInboundMessage(...) instead of repeating the ternary with sendInput/sendText so future changes (fields, validation, logging) live in one place.src/main/presenter/remoteControlPresenter/interface.ts (1)
16-23: Consider makingfilePresenterrequired to avoid silent media-feature downgrades.
filePresenteris the enabler for inbound attachment download into workspaces and generated-image persistence (core to this PR). Keeping it optional onRemoteControlPresenterDepsmeans a forgotten injection at any construction site will silently disable those flows with no compile-time signal. Sincesrc/main/presenter/index.tsalready injects it, consider tightening tofilePresenter: IFilePresenter— or, if optionality is required for a specific reason (e.g. tests), add an explicit runtime log/error when media arrives but the presenter is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/interface.ts` around lines 16 - 23, Change RemoteControlPresenterDeps so filePresenter is required (filePresenter: IFilePresenter) to prevent silent disabling of inbound attachment download and image persistence; update all places that construct or type RemoteControlPresenterDeps/RemoteControlPresenter (e.g., the RemoteControlPresenter constructor/usages) to pass an IFilePresenter instance. If making it required is not acceptable, instead add an explicit runtime guard in the RemoteControlPresenter code paths that handle inbound media or generated-image persistence to check for filePresenter and emit a clear error/log and short-circuit those flows when absent.src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkParser.ts (2)
71-95: Hardcoded.pngdefault extension may misrepresent non-PNG images.When
item.image_item.filenameis absent, the default isimage-${index + 1}.pngregardless of the actual content type. For image attachments whosecontent_typeisimage/jpegorimage/webp, this produces a file named.pngwith mismatched media type. Consider deriving the extension fromcontent_type.♻️ Proposed refactor
if (item.image_item) { - const filename = item.image_item.filename?.trim() || `image-${index + 1}.png` + const mediaType = item.image_item.content_type?.trim() || 'image/png' + const extension = mediaType.split('/')[1]?.split(';')[0]?.trim() || 'png' + const filename = item.image_item.filename?.trim() || `image-${index + 1}.${extension}` const encryptedMedia = extractEncryptedMedia( item.image_item.media, item.image_item.aeskey, item.image_item.aeskey ? 'hex' : 'auto' ) const url = normalizeText(item.image_item.url) const data = normalizeText(item.image_item.data) if (!url && !data && !encryptedMedia) { return null } return { id: `${index}`, filename, - mediaType: item.image_item.content_type?.trim() || 'image/png', + mediaType,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkParser.ts` around lines 71 - 95, The default filename in extractAttachmentFromItem uses a hardcoded `.png` when item.image_item.filename is missing; update this to derive the extension from item.image_item.content_type (e.g., map image/jpeg→.jpg, image/png→.png, image/webp→.webp, etc.) and fall back to `.png` only if content_type is unknown, then build the filename as `image-${index+1}${ext}`; ensure this change references the existing symbols item.image_item.filename and item.image_item.content_type and preserves the existing filename trimming behavior and return shape.
142-144: Redundant!commandcheck.
commandis derived exclusively fromtexton Line 142 (text ? parseCommand(text) : null), so whenevertextis empty,commandis guaranteed to benull. The!commandclause is therefore dead and can be removed for clarity.♻️ Proposed refactor
- if (!text && !command && attachments.length === 0) { + if (!text && attachments.length === 0) { return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkParser.ts` around lines 142 - 144, The condition checking for !command is redundant because command is set to parseCommand(text) only when text is truthy (const command = text ? parseCommand(text) : null), so remove the !command check from the if statement that currently reads if (!text && !command && attachments.length === 0) and simplify it to check only !text and attachments.length === 0 (i.e., update the conditional that guards empty input handling in the same block where parseCommand, command, text, and attachments are used).src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts (1)
621-636: Use injected logger and guard the fallbacksendMessage.Two small hygiene nits consistent with patterns elsewhere in this runtime:
console.warnbypasses the injectedthis.deps.logger?.error. Other catch branches (e.g., Lines 345–349) route throughdeps.loggerwith aconsole.errorfallback.- The fallback
this.deps.client.sendMessage(...)will re-throw on Discord API errors, aborting the rest of the image loop for this turn. Other error-path sends in this file use.catch(() => undefined)(e.g., Lines 360–362, Line 745) to keep delivery best-effort.♻️ Proposed refactor
for (const asset of snapshot.generatedImages ?? []) { try { await this.deps.client.sendImage(target.channelId, asset.path) } catch (error) { - console.warn('[DiscordRuntime] Failed to send generated image:', { - path: asset.path, - error - }) - await this.deps.client.sendMessage(target.channelId, `[Image]\nPath: ${asset.path}`) + const diagnostics = { path: asset.path, error } + if (this.deps.logger?.error) { + this.deps.logger.error('[DiscordRuntime] Failed to send generated image:', diagnostics) + } else { + console.warn('[DiscordRuntime] Failed to send generated image:', diagnostics) + } + await this.deps.client + .sendMessage(target.channelId, `[Image]\nPath: ${asset.path}`) + .catch(() => undefined) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts` around lines 621 - 636, In sendGeneratedImages, replace the direct console.warn with the injected logger (use this.deps.logger?.error(...) and fall back to console.warn if logger is missing) and ensure the fallback sendMessage call is best-effort by appending a catch to swallow errors (i.e., call this.deps.client.sendMessage(...).catch(() => undefined)); update references in the catch block that currently log asset.path and error to use this.deps.logger?.error for consistency (with console.warn fallback) and keep the rest of the loop intact so a failed sendMessage does not abort subsequent image sends.src/renderer/settings/components/RemoteSettings.vue (2)
176-251: Extract the default-workdir dropdown into a reusable component.The same ~70-line dropdown markup is duplicated five times (Telegram, Feishu, QQBot, Discord, Weixin iLink) with only the channel identifier differing. All five branches go through the same helpers (
directoryOptions,defaultWorkdirLabel,defaultWorkdirTitle,selectDefaultWorkdir,pickDefaultWorkdir,clearDefaultWorkdir,normalizePath). A single<RemoteDefaultWorkdirPicker :channel="'telegram'" />wrapping the DropdownMenu would collapse ~350 LOC and eliminate risk of per-channel drift when the UI is updated.Based on coding guidelines:
Vue components must use PascalCase naming (e.g., ChatInput.vue)— so aRemoteDefaultWorkdirPicker.vuecolocated alongside this file would be appropriate.Also applies to: 447-514, 659-726, 877-944, 1185-1254
🤖 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 176 - 251, The duplicated default-workdir DropdownMenu markup (used for channels like telegram, feishu, qqbot, discord, weixin iLink) should be extracted into a reusable PascalCase Vue component RemoteDefaultWorkdirPicker.vue; move the repeated template into this component and accept a prop "channel" (string), then call the existing helpers via the same names (directoryOptions(channel), defaultWorkdirLabel(channel), defaultWorkdirTitle(channel), selectDefaultWorkdir(channel, ...), pickDefaultWorkdir(channel), clearDefaultWorkdir(channel), normalizePath(...)) or expose any needed computed props/events so the parent RemoteSettings.vue simply renders <RemoteDefaultWorkdirPicker :channel="'telegram'"/> (and other channels) to replace each duplicated block, ensuring the component is colocated and uses the same class names and event handlers to preserve behavior and i18n labels.
2245-2255: Consider surfacing directory-picker errors to the user.
pickDefaultWorkdirswallows all errors fromprojectPresenter.selectDirectory()intoconsole.warn. If the IPC call fails (e.g., the presenter is unavailable or the dialog errors), the user gets no feedback — the dropdown just quietly does nothing. Other user-initiated actions in this file (e.g.,openBindingsDialog,generatePairCodeAndOpenDialog) surface failures viatoast({ variant: 'destructive' }). Aligning this path for consistency would improve UX.🤖 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 2245 - 2255, pickDefaultWorkdir currently swallows errors from projectPresenter.selectDirectory and only logs to console; change it to surface failures to the user via the app toast system: in the catch block of pickDefaultWorkdir, call the same toast pattern used in openBindingsDialog/generatePairCodeAndOpenDialog (toast({ variant: 'destructive', title: 'Failed to select directory', description: <error message> })) and still log the error; keep existing calls to setChannelDefaultWorkdir and loadRecentProjects unchanged when selection succeeds.src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts (1)
753-792: Use injected logger instead ofconsole.warnfor consistency.Other error paths in this runtime prefer
this.deps.logger?.error(...)with aconsole.errorfallback. The newsendGeneratedImagesbypasses that in favor of a directconsole.warn, which will be missed by centralized logging pipelines.♻️ Proposed refactor
} catch (error) { - console.warn('[QQBotRuntime] Failed to send generated image:', { - path: asset.path, - error - }) + const diagnostics = { path: asset.path, error } + if (this.deps.logger?.error) { + this.deps.logger.error('[QQBotRuntime] Failed to send generated image:', diagnostics) + } else { + console.warn('[QQBotRuntime] Failed to send generated image:', diagnostics) + } sendContext.sentCount -= 1 sendContext.nextMsgSeq -= 1 await this.sendText(sendContext, `[Image]\nPath: ${asset.path}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts` around lines 753 - 792, Replace the direct console.warn call in sendGeneratedImages with the injected logger: call this.deps.logger?.error(...) and if it's undefined fall back to console.error(...); keep the same message and contextual object (path and error) so the log reads similarly to the previous pattern, then roll back sendContext counters and call sendText as before. Target the sendGeneratedImages method and replace the console.warn usage with the logger fallback pattern used elsewhere in this class.src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts (1)
209-240: Dynamicimport('node:fs/promises')on every call; significant duplication betweenuploadC2CFileanduploadGroupFile.Both helpers are identical except for the endpoint path. Prefer a static top-level import and a single helper parameterized by path.
♻️ Proposed refactor
+import { readFile } from 'node:fs/promises' + type QQBotAccessTokenResponse = {- private async uploadC2CFile(openId: string, filePath: string): Promise<QQBotFileUploadResponse> { - const fileData = await import('node:fs/promises').then(async (fs) => - (await fs.readFile(filePath)).toString('base64') - ) - const response = await this.request(`/v2/users/${encodeURIComponent(openId)}/files`, { - method: 'POST', - body: JSON.stringify({ - file_type: 1, - file_data: fileData, - srv_send_msg: false - }) - }) - return (await response.json()) as QQBotFileUploadResponse - } - - private async uploadGroupFile( - groupOpenId: string, - filePath: string - ): Promise<QQBotFileUploadResponse> { - const fileData = await import('node:fs/promises').then(async (fs) => - (await fs.readFile(filePath)).toString('base64') - ) - const response = await this.request(`/v2/groups/${encodeURIComponent(groupOpenId)}/files`, { - method: 'POST', - body: JSON.stringify({ - file_type: 1, - file_data: fileData, - srv_send_msg: false - }) - }) - return (await response.json()) as QQBotFileUploadResponse - } + private async uploadFile( + targetPath: string, + filePath: string + ): Promise<QQBotFileUploadResponse> { + const fileData = (await readFile(filePath)).toString('base64') + const response = await this.request(targetPath, { + method: 'POST', + body: JSON.stringify({ + file_type: 1, + file_data: fileData, + srv_send_msg: false + }) + }) + return (await response.json()) as QQBotFileUploadResponse + }Then call
this.uploadFile(/v2/users/${encodeURIComponent(openId)}/files, filePath)etc. Similar consolidation possible forsendC2CImage/sendGroupImage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts` around lines 209 - 240, Both uploadC2CFile and uploadGroupFile duplicate logic and perform a dynamic import of node:fs/promises on every call; replace the dynamic imports with a static top-level import (e.g., import { readFile } from 'node:fs/promises') and consolidate the two methods into a single helper like uploadFile(path: string, filePath: string): Promise<QQBotFileUploadResponse> that reads the file, base64-encodes it, posts to the given endpoint, and returns the parsed response; then update uploadC2CFile and uploadGroupFile callers (or replace them) to call this.uploadFile(`/v2/users/${encodeURIComponent(openId)}/files`, filePath) and this.uploadFile(`/v2/groups/${encodeURIComponent(groupOpenId)}/files`, filePath) respectively — consider the same consolidation pattern for sendC2CImage/sendGroupImage.src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts (1)
77-90: Redundant early-return; consider consolidating.The Line 77 check
if (!rawText && attachments.length === 0)is a subset of the Line 88 checkif (!normalizedText && attachments.length === 0)sincenormalizedTextis derived fromrawTextby stripping mentions. Drop the first check and keep only the post-normalization one for clarity; behavior is equivalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts` around lines 77 - 90, Remove the redundant early-return that checks rawText and attachments before mention stripping: delete the `if (!rawText && attachments.length === 0)` block and rely on the later `const normalizedText = stripLeadingMentions(rawText)` followed by `if (!normalizedText && attachments.length === 0) { return null }`; this keeps logic around mentions (`mentions`, `botOpenId`) intact and avoids duplicate emptiness checks on `rawText` vs `normalizedText`.src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
321-328: Minor diagnostic inconsistency.The error-reply-fail
console.warnmixesparsed.chatId/threadIdwithmessage.messageId/eventId. SinceresolveMessageAttachmentsonly mutatesattachments, they are currently equivalent — but if the helper ever returns a differently keyed message the log would split identity between two sources. Use one consistently (prefermessage).🤖 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 321 - 328, The log in FeishuRuntime's error-reply block mixes parsed.chatId/threadId with message.messageId/eventId; update the console.warn to consistently use the message object (e.g., message.chatId and message.threadId) so identity comes from a single source (note resolveMessageAttachments may mutate attachments but use message for all IDs), and ensure message is defined before accessing its properties (guard if necessary).src/main/presenter/remoteControlPresenter/index.ts (1)
414-422:sanitizeDefaultAgentIdhas persistence side effects that run before the workdir assertion.
sanitizeDefaultAgentIdcan callbindingStore.updateTelegramConfig(...)(Line 1799-1805) to persist a newdefaultAgentId. IfassertAcpDefaultWorkdirthen throws, the agent-id mutation has already been committed while the rest of the save is rolled back. Same pattern applies to Feishu/QQBot/Discord/Weixin iLink save flows (lines 513-514, 577-578, 635-636, 698-699). Consider doing pure sanitization (no persistence) first, then assert, then apply all writes together.🤖 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 414 - 422, sanitizeDefaultAgentId is performing persistence (via bindingStore.updateTelegramConfig) before assertAcpDefaultWorkdir runs, causing partial commits on failure; refactor saveTelegramSettings so you first call a pure sanitizer that returns the sanitized defaultAgentId (avoid calling bindingStore.update* inside sanitizeDefaultAgentId), then call assertAcpDefaultWorkdir with that sanitized id, and only after assertions succeed perform all persistence in one step (e.g., update bindingStore.updateTelegramConfig and other writes together). Apply the same pattern to the Feishu/QQBot/Discord/Weixin iLink save flows referenced (ensure their sanitize* helpers are pure and delay bindingStore.update* until after workdir/assertion passes).test/main/presenter/remoteControlPresenter/weixinIlinkParser.test.ts (1)
1-48: LGTM.The test correctly exercises the encrypted image attachment path and its assertions align with
extractAttachmentFromItemandextractEncryptedMediainweixinIlinkParser.ts. Consider adding a follow-up case for thehd_sizefallback and amedia.aes_key-only path (noaeskey→aesKeyEncoding: 'auto') to tighten coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/weixinIlinkParser.test.ts` around lines 1 - 48, Add two additional test cases to WeixinIlinkParser tests: one exercising the hd_size fallback by providing an item with image_item.hd_size set (and no mid_size) and asserting the parsed attachment.size equals hd_size and resourceType/media fields match extractAttachmentFromItem behavior; and another where image_item has no top-level aeskey but media.aes_key is present so extractEncryptedMedia chooses aesKey from media.aes_key and sets aesKeyEncoding to 'auto' (assert encryptedMedia.aesKey === media.aes_key and aesKeyEncoding === 'auto'); implement both tests alongside the existing one using WeixinIlinkParser.parseMessage to construct messages and assert attachments accordingly.test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts (1)
35-38: Clean upfs.mkdtempworkspaces inafterEach.The new tests create workspace directories at Lines 129 and 197 but never remove them, so each CI run leaves orphan
deepchat-remote-runner-*directories underos.tmpdir(). Track the paths and remove them in the existingafterEach:♻️ Proposed cleanup
+const tempWorkspaces: string[] = [] + describe('RemoteConversationRunner', () => { - afterEach(() => { + afterEach(async () => { vi.unstubAllGlobals() + await Promise.all( + tempWorkspaces.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })) + ) })Then push each created workspace into
tempWorkspacesafterfs.mkdtemp(...)at Lines 129 and 197.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts` around lines 35 - 38, Add a tempWorkspaces array in the test file scope, push each created workspace path returned from fs.mkdtemp into tempWorkspaces where those two workspaces are created, and update the existing afterEach (the vi.unstubAllGlobals block) to iterate tempWorkspaces and remove each directory (e.g., using fs.rmSync or fs.promises.rm with recursive/force) so the deepchat-remote-runner-* temp dirs created by fs.mkdtemp are cleaned up after each test.
🤖 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/remoteControlPresenter/adapters/feishu/FeishuAdapter.ts`:
- Around line 121-130: FeishuAdapter.sendImage currently discards the result of
FeishuClient.sendImage (which returns Promise<string | null>), hiding soft
failures; update the adapter to propagate the send result by changing sendImage
to return Promise<string> (or otherwise propagate the string) and after awaiting
this.client.sendImage(...) check for null and throw an Error when message_id is
missing; reference FeishuAdapter.sendImage and FeishuClient.sendImage and ensure
callers receive the message id (or an exception) instead of a silent success.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 227-231: The return value from the Feishu download function is
hardcoding mediaType to 'image/png' or 'application/octet-stream', which
overrides real MIME types; update the download function in feishuClient.ts (the
attachment/resource download function that builds the {data, mediaType,
filename} result) to read the response Content-Type header and set mediaType to
that value when present, otherwise set mediaType to undefined so the caller can
preserve the parser-provided MIME; also remove or stop populating the unused
filename field from the function's return signature (and adjust its type).
Finally, update feishuRuntime.ts where it currently does mediaType:
downloaded.mediaType || attachment.mediaType to prefer attachment.mediaType when
downloaded.mediaType is undefined or empty (or flip the merge order) so original
MIME types are preserved.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts`:
- Around line 54-60: The synthetic filename and mediaType are hardcoded to PNG;
change the attachment object in feishuParser.ts to avoid a misleading .png
extension and hardcoded 'image/png'—use filename: content.image_key (no
extension) or filename: `${content.image_key}` and remove or omit the mediaType
property (or set it to undefined) so feishuRuntime.resolveMessageAttachments can
overwrite the real MIME/type and the filename isn't incorrectly tied to PNG;
keep resourceKey: content.image_key and resourceType: 'image' as-is.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 332-371: In resolveMessageAttachments, detect download failures
and don't silently return the original attachment; instead tag failed
attachments so downstream can act or surface a reply: when
downloadMessageResource in resolveMessageAttachments fails for an attachment,
return the attachment augmented with a clear failure marker (e.g.
failedDownload: true and errorMessage: 'Failed to load attachment') and leave
data undefined, and after Promise.all check if every attachment has failed (all
failedDownload === true) and set a flag on the returned FeishuInboundMessage
(e.g. allAttachmentsFailed: true) so calling code can send "Failed to load your
attachment — please resend." or handle accordingly. Ensure you reference
resolveMessageAttachments, FeishuInboundMessage, attachments, and
downloadMessageResource in the change.
In `@src/main/presenter/remoteControlPresenter/index.ts`:
- Around line 1837-1851: The typeof check for getAgentType in
assertAcpDefaultWorkdir is dead code because IConfigPresenter guarantees
getAgentType exists; remove the guard and call
this.deps.configPresenter.getAgentType(agentId) directly (or if you prefer
explicit runtime failure, replace the typeof check with a throw that states the
required presenter method is missing). Ensure you still return early when the
agent type is not 'acp' and keep the final check that throws when defaultWorkdir
is empty.
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotParser.ts`:
- Around line 42-54: normalizeAttachments currently assigns a static default
filename 'attachment' which can collide when multiple attachments lack a
filename; update the map inside normalizeAttachments to accept the map index and
produce a unique default like `attachment-${index + 1}` (or `attachment-${index
+ 1}${ext}` if you want to preserve an extension parsed from attachment.url or
content_type) whenever attachment.filename is falsy, so each normalized
attachment gets a distinct filename and avoids downstream overwrites.
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts`:
- Around line 766-790: The fallback sendText call inside the image-send catch
can re-throw after sendText has already incremented
sendContext.sentCount/sendContext.nextMsgSeq (see sendText and sendContext),
which can leak msgSeq state and trigger the outer error handler in
processInboundMessage to send QQBOT_INTERNAL_ERROR_REPLY and possibly exceed
QQBOT_MAX_PASSIVE_REPLIES; fix by making the fallback call non-throwing — wrap
the fallback this.sendText(sendContext, `[Image]\nPath: ${asset.path}`) in a
promise-safe swallow (e.g., append .catch(() => undefined)) so any error during
the fallback won’t propagate and corrupt sendContext/msgSeq state or trigger the
outer error path that calls deliverConversation/processInboundMessage handlers.
In
`@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Around line 867-930: The persistGeneratedImages method currently calls
fs.mkdir and fs.writeFile without error handling which can crash the snapshot
flow; update persistGeneratedImages so that the workspace/assetDir creation
(fs.mkdir) is wrapped in a try/catch and returns [] or skips image persistence
on failure, and also wrap each per-block file write (fs.writeFile) in its own
try/catch so a single failing image is skipped (log a warning with
endpointKey/messageId/index and error) while still returning any successfully
persisted RemoteGeneratedImageAsset entries; reference persistGeneratedImages,
resolveAssetWorkspace, fs.mkdir, fs.writeFile, and RemoteGeneratedImageAsset
when locating where to add the guards.
- Around line 650-658: The fetch calls in prepareRemoteAttachments (used by
sendInput via remoteConversationRunner) have no timeout and can hang; update
both fetch(url) invocations (the plain download and the encrypted download path
inside prepareRemoteAttachments) to pass an AbortSignal created with
AbortSignal.timeout(desiredMs) so a stalled request throws a TimeoutError that
will propagate via the existing throw handling; ensure you import or reference
AbortSignal and pick a sensible timeout constant, and reuse the same timeout for
both fetch calls so prepareRemoteAttachments fails fast instead of blocking
sendInput.
- Around line 64-87: sanitizePathSegment and sanitizeFileName currently allow
"." and ".." to pass through (since dots aren't removed), enabling
path-traversal via path.basename; update sanitizePathSegment to treat "." and
".." (and empty results) as invalid and return the fallback instead of the
sanitized value, and update sanitizeFileName to re-check the computed baseName
for "." or ".." (or empty string) and replace it with the fallback before using
path.extname or appending MIME extensions; reference sanitizePathSegment,
sanitizeFileName, path.basename, path.extname and ensure callers that join with
assetDir/path.join will never receive "."/".." segments so writes won't escape
or collide with sibling directories.
In `@src/main/presenter/remoteControlPresenter/telegram/telegramClient.ts`:
- Around line 156-176: The downloadFileBase64 method lacks a timeout and can
hang; update downloadFileBase64 to perform the file GET with an AbortSignal
timeout (e.g. AbortSignal.timeout(35_000)) or route the call through the
existing request() helper so the same timeout/abort behavior is applied; locate
downloadFileBase64, use getFile to obtain filePath, then call
fetch(`${this.fileBaseUrl}/${filePath}`, { signal: AbortSignal.timeout(35000) })
(or equivalent request() invocation) and keep the same error handling and Buffer
conversion.
- Around line 178-211: The sendPhoto method currently does an unbounded fetch
and uses dynamic imports/split for file handling; fix it by importing
fs/promises and path.basename statically at the top of the module and replacing
the dynamic import and manual split with fs.readFile(filePath) and
path.basename(filePath) respectively, and add an AbortController with a timeout
(matching the pattern used in downloadFileBase64) to the fetch call: create the
controller, pass controller.signal to fetch, set a timer to abort after the
configured timeout, and clear the timer on success/failure so uploads can't hang
indefinitely.
In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.ts`:
- Around line 588-596: The fallback currently sends the absolute local path
(asset.path) to remote users in weixinIlinkRuntime.ts via sendText; change the
sendText fallback call in the catch block of the image send routine to a neutral
message (e.g., "[Image] Delivery failed — see local copy in the app.") and
ensure asset.path is only kept in internal logs via this.logInfo (do not include
it in any message sent to remote endpoints). Apply the same fix to the
equivalent fallback sites in feishuRuntime.ts (the same sendText/send fallback)
and telegramPoller.ts so no absolute local paths are echoed to remote chats.
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Line 2172: The pathLabel helper returns an empty string for inputs with a
trailing separator because value.split(/[/\\]/).pop() can be '' (not
null/undefined) and the ?? fallback won't run; update pathLabel to ignore empty
segments (e.g., filter(Boolean) before popping) or strip trailing separators
first, then take the last segment, and if that final segment is an empty string
still fall back to the original value; reference: pathLabel(value: string) and
its use in the dropdown label/button.
In `@src/renderer/src/i18n/zh-CN/settings.json`:
- Around line 1723-1724: Update the two i18n keys defaultWorkdirPlaceholder and
defaultWorkdirHelper in the remaining locale files (zh-TW, ja-JP, zh-HK, ru-RU,
ko-KR, he-IL, fr-FR, pt-BR, fa-IR, da-DK) so they match the new zh-CN/en-US
semantics: placeholder should indicate "Select default directory" and helper
should state that this is the working directory for remote-control sessions and
is required when the default agent is ACP (i.e., replace the old "leave empty to
use global default" wording). Locate and edit the entries named
defaultWorkdirPlaceholder and defaultWorkdirHelper in each of those locale JSON
files and update their strings to convey the ACP-required semantics.
---
Outside diff comments:
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts`:
- Around line 140-207: The upload response may omit file_info
(QQBotFileUploadResponse has it optional), so in sendC2CImage and
sendGroupImage, after calling uploadC2CFile(target.openId, target.filePath) and
uploadGroupFile(target.groupOpenId, target.filePath) respectively, validate that
media.file_info exists and throw a clear error (e.g., throw new Error('QQBot
image upload did not return file_info.')) to fail fast instead of posting media
with undefined file_info; reference sendC2CImage, sendGroupImage, uploadC2CFile,
uploadGroupFile, and the file_info field when adding this check.
In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.ts`:
- Around line 528-561: The method appendTerminalDeliverySegment in
weixinIlinkRuntime.ts is unused and exposed publicly; remove the dead code by
deleting the entire appendTerminalDeliverySegment implementation (or, if you
intend to keep it for internal reuse, change its declaration to private and
ensure any test-only calls are removed), matching other runtimes that declare
this helper as private (e.g., telegram/feishu/discord); update any
imports/usages if present to avoid dangling references.
---
Duplicate comments:
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 253-288: The resolveMessageAttachments method currently leaves
attachments without data silent on downloadFileBase64 failures; update the catch
block to annotate the attachment with a download error and surface that to the
returned TelegramInboundMessage so callers can detect failures. Specifically,
when downloadFileBase64 (used in TelegramPoller.resolveMessageAttachments)
throws, return the original attachment augmented with a machine-readable error
field (e.g., downloadError: string) and also set a message-level indicator on
the returned TelegramInboundMessage (e.g., failedAttachments: array of filenames
or a boolean attachmentsFailed) so downstream code can notify the user or handle
retries; keep the existing console.warn but include the error.message text in
the downloadError value.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/discord/discordClient.ts`:
- Around line 141-142: Replace the dynamic import and manual path splitting:
statically import fs/promises (used by fileBuffer assignment) at the top of the
module and use path.basename from 'path' to compute fileName (replace the
fileName = filePath.split... line) so you avoid dynamic import overhead and
handle edge cases like trailing separators; update references to fileBuffer and
fileName accordingly (look for the fileBuffer assignment and fileName variable
in discordClient.ts).
In `@src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts`:
- Around line 621-636: In sendGeneratedImages, replace the direct console.warn
with the injected logger (use this.deps.logger?.error(...) and fall back to
console.warn if logger is missing) and ensure the fallback sendMessage call is
best-effort by appending a catch to swallow errors (i.e., call
this.deps.client.sendMessage(...).catch(() => undefined)); update references in
the catch block that currently log asset.path and error to use
this.deps.logger?.error for consistency (with console.warn fallback) and keep
the rest of the loop intact so a failed sendMessage does not abort subsequent
image sends.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 190-198: The declared optional filename in
downloadMessageResource's Promise return is never set; either remove filename
from the return type or populate it (e.g., extract from the SDK response
metadata or derive from params.fileKey) and return it alongside data and
mediaType; update any caller such as feishuRuntime.ts if it needs to consume the
filename (or remove its expectations) so the function signature and actual
returned object stay consistent.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts`:
- Around line 77-90: Remove the redundant early-return that checks rawText and
attachments before mention stripping: delete the `if (!rawText &&
attachments.length === 0)` block and rely on the later `const normalizedText =
stripLeadingMentions(rawText)` followed by `if (!normalizedText &&
attachments.length === 0) { return null }`; this keeps logic around mentions
(`mentions`, `botOpenId`) intact and avoids duplicate emptiness checks on
`rawText` vs `normalizedText`.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 321-328: The log in FeishuRuntime's error-reply block mixes
parsed.chatId/threadId with message.messageId/eventId; update the console.warn
to consistently use the message object (e.g., message.chatId and
message.threadId) so identity comes from a single source (note
resolveMessageAttachments may mutate attachments but use message for all IDs),
and ensure message is defined before accessing its properties (guard if
necessary).
In `@src/main/presenter/remoteControlPresenter/index.ts`:
- Around line 414-422: sanitizeDefaultAgentId is performing persistence (via
bindingStore.updateTelegramConfig) before assertAcpDefaultWorkdir runs, causing
partial commits on failure; refactor saveTelegramSettings so you first call a
pure sanitizer that returns the sanitized defaultAgentId (avoid calling
bindingStore.update* inside sanitizeDefaultAgentId), then call
assertAcpDefaultWorkdir with that sanitized id, and only after assertions
succeed perform all persistence in one step (e.g., update
bindingStore.updateTelegramConfig and other writes together). Apply the same
pattern to the Feishu/QQBot/Discord/Weixin iLink save flows referenced (ensure
their sanitize* helpers are pure and delay bindingStore.update* until after
workdir/assertion passes).
In `@src/main/presenter/remoteControlPresenter/interface.ts`:
- Around line 16-23: Change RemoteControlPresenterDeps so filePresenter is
required (filePresenter: IFilePresenter) to prevent silent disabling of inbound
attachment download and image persistence; update all places that construct or
type RemoteControlPresenterDeps/RemoteControlPresenter (e.g., the
RemoteControlPresenter constructor/usages) to pass an IFilePresenter instance.
If making it required is not acceptable, instead add an explicit runtime guard
in the RemoteControlPresenter code paths that handle inbound media or
generated-image persistence to check for filePresenter and emit a clear
error/log and short-circuit those flows when absent.
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts`:
- Around line 209-240: Both uploadC2CFile and uploadGroupFile duplicate logic
and perform a dynamic import of node:fs/promises on every call; replace the
dynamic imports with a static top-level import (e.g., import { readFile } from
'node:fs/promises') and consolidate the two methods into a single helper like
uploadFile(path: string, filePath: string): Promise<QQBotFileUploadResponse>
that reads the file, base64-encodes it, posts to the given endpoint, and returns
the parsed response; then update uploadC2CFile and uploadGroupFile callers (or
replace them) to call
this.uploadFile(`/v2/users/${encodeURIComponent(openId)}/files`, filePath) and
this.uploadFile(`/v2/groups/${encodeURIComponent(groupOpenId)}/files`, filePath)
respectively — consider the same consolidation pattern for
sendC2CImage/sendGroupImage.
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts`:
- Around line 753-792: Replace the direct console.warn call in
sendGeneratedImages with the injected logger: call this.deps.logger?.error(...)
and if it's undefined fall back to console.error(...); keep the same message and
contextual object (path and error) so the log reads similarly to the previous
pattern, then roll back sendContext counters and call sendText as before. Target
the sendGeneratedImages method and replace the console.warn usage with the
logger fallback pattern used elsewhere in this class.
In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts`:
- Around line 211-226: Duplicate conditional dispatch of attachments vs text
across routers should be consolidated into a single helper on the runner (e.g.,
add a method like sendFromInboundMessage) that accepts {endpointKey, text,
attachments, sourceMessageId, bindingMeta}, performs the attachments.length > 0
check and calls the existing sendInput or sendText accordingly (preserving
sourceMessageId coercion logic currently used in feishuCommandRouter.ts), and
returns the same response shape; update feishuCommandRouter.ts (and the other
routers) to call deps.runner.sendFromInboundMessage(...) instead of repeating
the ternary with sendInput/sendText so future changes (fields, validation,
logging) live in one place.
In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkParser.ts`:
- Around line 71-95: The default filename in extractAttachmentFromItem uses a
hardcoded `.png` when item.image_item.filename is missing; update this to derive
the extension from item.image_item.content_type (e.g., map image/jpeg→.jpg,
image/png→.png, image/webp→.webp, etc.) and fall back to `.png` only if
content_type is unknown, then build the filename as `image-${index+1}${ext}`;
ensure this change references the existing symbols item.image_item.filename and
item.image_item.content_type and preserves the existing filename trimming
behavior and return shape.
- Around line 142-144: The condition checking for !command is redundant because
command is set to parseCommand(text) only when text is truthy (const command =
text ? parseCommand(text) : null), so remove the !command check from the if
statement that currently reads if (!text && !command && attachments.length ===
0) and simplify it to check only !text and attachments.length === 0 (i.e.,
update the conditional that guards empty input handling in the same block where
parseCommand, command, text, and attachments are used).
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Around line 176-251: The duplicated default-workdir DropdownMenu markup (used
for channels like telegram, feishu, qqbot, discord, weixin iLink) should be
extracted into a reusable PascalCase Vue component
RemoteDefaultWorkdirPicker.vue; move the repeated template into this component
and accept a prop "channel" (string), then call the existing helpers via the
same names (directoryOptions(channel), defaultWorkdirLabel(channel),
defaultWorkdirTitle(channel), selectDefaultWorkdir(channel, ...),
pickDefaultWorkdir(channel), clearDefaultWorkdir(channel), normalizePath(...))
or expose any needed computed props/events so the parent RemoteSettings.vue
simply renders <RemoteDefaultWorkdirPicker :channel="'telegram'"/> (and other
channels) to replace each duplicated block, ensuring the component is colocated
and uses the same class names and event handlers to preserve behavior and i18n
labels.
- Around line 2245-2255: pickDefaultWorkdir currently swallows errors from
projectPresenter.selectDirectory and only logs to console; change it to surface
failures to the user via the app toast system: in the catch block of
pickDefaultWorkdir, call the same toast pattern used in
openBindingsDialog/generatePairCodeAndOpenDialog (toast({ variant:
'destructive', title: 'Failed to select directory', description: <error message>
})) and still log the error; keep existing calls to setChannelDefaultWorkdir and
loadRecentProjects unchanged when selection succeeds.
In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts`:
- Around line 90-127: Add a new unit test covering the parser's file branch:
call FeishuParser.parseEvent with message.message_type === 'file' and content
JSON including file_key and file_name cases (both non-empty and whitespace-only)
to verify attachments use filename fallback logic (file_name.trim() || file_key)
and that mediaType is 'application/octet-stream'; assert parsed.text is '' and
attachments contain id/resourceKey equal to file_key and filename set to either
trimmed file_name or file_key when file_name is blank.
In `@test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts`:
- Around line 35-38: Add a tempWorkspaces array in the test file scope, push
each created workspace path returned from fs.mkdtemp into tempWorkspaces where
those two workspaces are created, and update the existing afterEach (the
vi.unstubAllGlobals block) to iterate tempWorkspaces and remove each directory
(e.g., using fs.rmSync or fs.promises.rm with recursive/force) so the
deepchat-remote-runner-* temp dirs created by fs.mkdtemp are cleaned up after
each test.
In `@test/main/presenter/remoteControlPresenter/weixinIlinkParser.test.ts`:
- Around line 1-48: Add two additional test cases to WeixinIlinkParser tests:
one exercising the hd_size fallback by providing an item with image_item.hd_size
set (and no mid_size) and asserting the parsed attachment.size equals hd_size
and resourceType/media fields match extractAttachmentFromItem behavior; and
another where image_item has no top-level aeskey but media.aes_key is present so
extractEncryptedMedia chooses aesKey from media.aes_key and sets aesKeyEncoding
to 'auto' (assert encryptedMedia.aesKey === media.aes_key and aesKeyEncoding ===
'auto'); implement both tests alongside the existing one using
WeixinIlinkParser.parseMessage to construct messages and assert attachments
accordingly.
🪄 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: 39f5713a-1b80-4967-9f65-e978a70d9184
📒 Files selected for processing (45)
docs/specs/remote-block-streaming/spec.mddocs/specs/remote-multi-channel/spec.mdsrc/main/presenter/index.tssrc/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/feishu/FeishuAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/qqbot/QQBotAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/weixinIlink/WeixinIlinkAdapter.tssrc/main/presenter/remoteControlPresenter/discord/discordClient.tssrc/main/presenter/remoteControlPresenter/discord/discordRuntime.tssrc/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/feishu/feishuParser.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/interface.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotClient.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotParser.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tssrc/main/presenter/remoteControlPresenter/services/discordCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/qqbotCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/services/weixinIlinkCommandRouter.tssrc/main/presenter/remoteControlPresenter/telegram/telegramClient.tssrc/main/presenter/remoteControlPresenter/telegram/telegramParser.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/remoteControlPresenter/types.tssrc/main/presenter/remoteControlPresenter/types/channel.tssrc/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkClient.tssrc/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkParser.tssrc/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.tssrc/renderer/settings/components/RemoteSettings.vuesrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/shared/types/presenters/remote-control.presenter.d.tstest/main/presenter/remoteControlPresenter/discordParser.test.tstest/main/presenter/remoteControlPresenter/feishuParser.test.tstest/main/presenter/remoteControlPresenter/qqbotAdapter.test.tstest/main/presenter/remoteControlPresenter/qqbotParser.test.tstest/main/presenter/remoteControlPresenter/remoteBlockRenderer.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.tstest/main/presenter/remoteControlPresenter/telegramParser.test.tstest/main/presenter/remoteControlPresenter/weixinIlinkParser.test.ts
Comment on lines
+588
to
+596
| } catch (error) { | ||
| this.logInfo('Failed to send Weixin iLink generated image; sending local path.', { | ||
| accountId: this.deps.accountId, | ||
| path: asset.path, | ||
| error: error instanceof Error ? error.message : String(error) | ||
| }) | ||
| await this.sendText(sendContext, `[Image]\nPath: ${asset.path}`, 'image-fallback') | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Local filesystem path leaked to remote chat users on image send failure.
asset.path is an absolute local path (likely under the user's profile/app-data directory). Echoing it to a remote chat exposes the OS username and internal storage layout to whoever controls the remote endpoint, with no benefit—the recipient can't access that path. The same fallback pattern exists in feishuRuntime.ts:596-601 and telegramPoller.ts:552-560, so consider fixing uniformly. A neutral message (e.g., '[Image] Delivery failed — see local copy in the app.') preserves the signal without leaking PII.
🛡️ Proposed change
- await this.sendText(sendContext, `[Image]\nPath: ${asset.path}`, 'image-fallback')
+ await this.sendText(
+ sendContext,
+ '[Image] Unable to deliver the generated image to this channel.',
+ 'image-fallback'
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.ts`
around lines 588 - 596, The fallback currently sends the absolute local path
(asset.path) to remote users in weixinIlinkRuntime.ts via sendText; change the
sendText fallback call in the catch block of the image send routine to a neutral
message (e.g., "[Image] Delivery failed — see local copy in the app.") and
ensure asset.path is only kept in internal logs via this.logInfo (do not include
it in any message sent to remote endpoints). Apply the same fix to the
equivalent fallback sites in feishuRuntime.ts (the same sendText/send fallback)
and telegramPoller.ts so no absolute local paths are echoed to remote chats.
Comment on lines
+1723
to
+1724
| "defaultWorkdirPlaceholder": "选择默认目录", | ||
| "defaultWorkdirHelper": "远控会话的工作目录。默认智能体为 ACP 时必须选择。", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how each locale renders the two new keys for the remote-control defaultWorkdir.
fd -e json 'settings.json' src/renderer/src/i18n --exec sh -c '
echo "=== $1 ==="
rg -n "defaultWorkdirPlaceholder|defaultWorkdirHelper" "$1" || echo "(missing keys)"
' sh {}Repository: ThinkInAIXYZ/deepchat
Length of output: 2971
All other locales (10 files) still contain outdated copy that contradicts the new zh-CN and en-US semantics.
Only zh-CN and en-US have been updated with the new ACP-required copy at lines 1723–1724. All other locales (zh-TW, ja-JP, zh-HK, ru-RU, ko-KR, he-IL, fr-FR, pt-BR, fa-IR, da-DK) still have the old fallback-based wording at lines 1665–1666 ("leave empty to use global default"). This creates contradictory UX: Chinese and English users see "required when default agent is ACP," while all other users see "leave empty to fallback." Update all 10 locale files to match the zh-CN/en-US semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/i18n/zh-CN/settings.json` around lines 1723 - 1724, Update
the two i18n keys defaultWorkdirPlaceholder and defaultWorkdirHelper in the
remaining locale files (zh-TW, ja-JP, zh-HK, ru-RU, ko-KR, he-IL, fr-FR, pt-BR,
fa-IR, da-DK) so they match the new zh-CN/en-US semantics: placeholder should
indicate "Select default directory" and helper should state that this is the
working directory for remote-control sessions and is required when the default
agent is ACP (i.e., replace the old "leave empty to use global default"
wording). Locate and edit the entries named defaultWorkdirPlaceholder and
defaultWorkdirHelper in each of those locale JSON files and update their strings
to convey the ACP-required semantics.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/index.ts (1)
1649-1666:⚠️ Potential issue | 🟠 MajorInclude Weixin iLink
defaultWorkdirin the adapter signature too.This signature is what decides whether
rebuildWeixinIlinkAccountRuntime()reuses the existing connected adapter. SincedefaultWorkdiris missing here, changing only that setting won't invalidate the runtime for Weixin iLink, unlike the other built-in channels.♻️ Suggested change
private buildWeixinIlinkAdapterSignature( defaultAgentId: string, + defaultWorkdir: string, account: { accountId: string ownerUserId: string baseUrl: string botToken: string @@ return JSON.stringify({ accountId: account.accountId, ownerUserId: account.ownerUserId, baseUrl: account.baseUrl, botToken: account.botToken, enabled: account.enabled, - defaultAgentId: defaultAgentId.trim() + defaultAgentId: defaultAgentId.trim(), + defaultWorkdir: defaultWorkdir.trim() }) }You'll also need to thread the new argument through the existing call sites.
🤖 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 1649 - 1666, The adapter signature built by buildWeixinIlinkAdapterSignature is missing defaultWorkdir, so include a defaultWorkdir field in the returned JSON (e.g., "defaultWorkdir": defaultWorkdir.trim()) and ensure the function gains a defaultWorkdir parameter; then update every call site that invokes buildWeixinIlinkAdapterSignature to pass the corresponding defaultWorkdir value so that rebuildWeixinIlinkAccountRuntime() will detect changes to defaultWorkdir and recreate the adapter runtime accordingly.
🧹 Nitpick comments (5)
test/main/presenter/remoteControlPresenter/feishuAdapter.test.ts (1)
124-149: Consider asserting the transport target forwarded to the client.The test only checks that
clientInstances[0].sendImagewas called. It won't catch regressions where the adapter dropsthreadId/replyToMessageIdwhen parsing'oc_1:root'. AtoHaveBeenCalledWith(expect.objectContaining({ chatId: 'oc_1', ... }), '/tmp/generated.png')assertion would tighten coverage and complement the image delivery path being added in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuAdapter.test.ts` around lines 124 - 149, The test for FeishuAdapter.sendImage only asserts that clientInstances[0].sendImage was called but doesn't verify the transport payload; update the test to assert the adapter forwards the correct target (parse 'oc_1:root' into chatId/threadId/replyToMessageId as appropriate) by adding a toHaveBeenCalledWith(expect.objectContaining({ chatId: 'oc_1', threadId: 'root', replyToMessageId: /* expected if any */ }), '/tmp/generated.png') check so you ensure FeishuAdapter.sendImage builds and passes the correct transport target to clientInstances[0].sendImage.src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts (1)
86-90: Optional: surface the text alongside the attachment-failure reply.When a user sends text and an image that fails to download, this short-circuit drops the text entirely and only asks them to resend. In practice Feishu image messages rarely carry text, so this is fine for now — but a future improvement could either forward the text portion to the runner or include it in the error reply so the user doesn't retype both. No action required for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts` around lines 86 - 90, The early return on message.allAttachmentsFailed drops any accompanying text; update the handler that checks message.allAttachmentsFailed to preserve/forward message.text (or include it in the error reply) instead of discarding it: when message.allAttachmentsFailed is true, construct the returned object to include both replies: ['Failed to load your attachment. Please resend.'] and also include the original message.text (e.g., append it to replies or set a forwardedText field) so downstream logic (or the user-facing reply) retains the user's text; locate the check for message.allAttachmentsFailed in feishuCommandRouter.ts to implement this change.test/main/presenter/remoteControlPresenter/feishuParser.test.ts (1)
93-111: Nit: passbotOpenIdtoparseEventfor consistency.Other tests in this file call
parser.parseEvent(event, 'ou_bot'). Omitting the bot open id here is inconsistent and will hide any future regression where the parser considers the bot id for image/attachment messages (e.g., inferring whether the upload was forwarded by the bot itself).♻️ Suggested adjustment
- const parsed = parser.parseEvent({ + const parsed = parser.parseEvent( + { event_id: 'evt-3', ... - }) - } - }) + }) + }, + 'ou_bot' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts` around lines 93 - 111, The test calls parser.parseEvent without the botOpenId causing inconsistency with other tests; update the call to include the bot open id (e.g., 'ou_bot') so parseEvent(parser.parseEvent(...), or rather parser.parseEvent(event, 'ou_bot')) is invoked with the same second-argument signature used elsewhere, ensuring the parser receives the botOpenId when processing the image message; locate the invocation of parseEvent in this test (the parser.parseEvent(...) around the image message) and add the botOpenId parameter.src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
594-611:sendGeneratedImagesloop: fallbacksendTextfailure will abort remaining images.If
this.deps.client.sendText(...)inside the catch also fails (e.g., transient Feishu outage), the exception escapes thefor…ofand any remaining images are silently dropped. Consider wrapping the fallbacksendTextin its owntry/catchso per-image failures stay local to that iteration.♻️ Proposed refactor
for (const asset of snapshot.generatedImages ?? []) { try { await this.deps.client.sendImage(target, asset.path) } catch (error) { console.warn('[FeishuRuntime] Failed to send generated image:', { path: asset.path, error }) - await this.deps.client.sendText( - target, - '[Image] Delivery failed - see local copy in the app.' - ) + try { + await this.deps.client.sendText( + target, + '[Image] Delivery failed - see local copy in the app.' + ) + } catch (fallbackError) { + console.warn('[FeishuRuntime] Failed to send image-fallback text:', fallbackError) + } } }The same pattern applies to
weixinIlinkRuntime.tsandtelegramPoller.ts.🤖 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 594 - 611, In sendGeneratedImages, the catch block around this.deps.client.sendImage currently calls this.deps.client.sendText directly which can throw and break the for…of loop; wrap the fallback sendText call in its own try/catch inside the same iteration so any failure to send the fallback message is logged (e.g., with console.warn or processLogger) and does not abort delivery of remaining assets—apply the same pattern to the analogous handlers in weixinIlinkRuntime.ts and telegramPoller.ts.test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts (1)
128-284: Temp workspace directories created by tests are never cleaned up.Each
fs.mkdtemp(...)creates a fresh directory under the OS tmp dir but the tests never remove them. Not critical (CI usually reaps/tmp), but tracking them andfs.rm(..., { recursive: true, force: true })inafterEachkeeps local test runs tidy.♻️ Optional refactor
describe('RemoteConversationRunner', () => { + const tempDirs: string[] = [] + afterEach(() => { vi.unstubAllGlobals() }) + + afterEach(async () => { + await Promise.all( + tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })) + ) + })Then push each
mkdtempresult intotempDirsso they can be cleaned up uniformly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts` around lines 128 - 284, Tests in remoteConversationRunner.test.ts create temporary workspaces via fs.mkdtemp and never remove them; collect each mkdtemp result (e.g., the workspace variables created in the two it blocks) into a shared tempDirs array and add an afterEach(async () => { await Promise.all(tempDirs.map(d => fs.rm(d, { recursive: true, force: true }))) }) to remove them; ensure you push the workspace variable used to construct preparedFile into tempDirs after creation and use await fs.rm with recursive:true and force:true so cleanup runs reliably between tests.
🤖 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/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 276-302: Update sendImage to mirror sendText/sendCard routing:
check target.replyToMessageId and if present call this.sdk.im.message.reply
(including params.receive_id_type='chat_id', data.receive_id=target.chatId,
data.msg_type='image', data.content with image_key, and set reply_in_thread:
Boolean(target.threadId)), otherwise call this.sdk.im.message.create as
currently implemented; preserve the existing return behavior using
FeishuMessageResponse.data.message_id. Before creating the fs.createReadStream,
validate imagePath with fs.promises.access (or equivalent) and throw a clear
error if the file is missing to fail fast instead of relying on the SDK upload
error.
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts`:
- Around line 703-713: The helper getFinalDeliveryText currently suppresses
fallback text whenever snapshot.generatedImages exists, causing captions/answers
in snapshot.fullText or snapshot.text to be lost; change the logic in
getFinalDeliveryText so that it only returns an empty string for image-only
deliveries when finalText is empty AND both snapshot.fullText and snapshot.text
are empty/whitespace, otherwise fall back to return (snapshot.fullText ??
snapshot.text).trim(); update references to snapshot.finalText,
snapshot.generatedImages, snapshot.fullText and snapshot.text in that function
to implement this conditional.
In
`@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Around line 608-643: prepareRemoteAttachments currently fails the whole upload
when any attachment lacks downloadable data; update it to skip attachments where
attachment.failedDownload === true or none of
attachment.data/attachment.url/attachment.encryptedMedia exist, continuing with
the rest: before writing files and calling readAttachmentData, filter or
conditionalize each attachment in prepareRemoteAttachments (referencing function
prepareRemoteAttachments and readAttachmentData) to if-miss -> log a warning (or
collect a warning list on the session/input) and continue rather than calling
readAttachmentData (so only attachments with downloadable data are written and
passed to prepareMessageFile); ensure the returned MessageFile[] contains only
successfully prepared files.
---
Outside diff comments:
In `@src/main/presenter/remoteControlPresenter/index.ts`:
- Around line 1649-1666: The adapter signature built by
buildWeixinIlinkAdapterSignature is missing defaultWorkdir, so include a
defaultWorkdir field in the returned JSON (e.g., "defaultWorkdir":
defaultWorkdir.trim()) and ensure the function gains a defaultWorkdir parameter;
then update every call site that invokes buildWeixinIlinkAdapterSignature to
pass the corresponding defaultWorkdir value so that
rebuildWeixinIlinkAccountRuntime() will detect changes to defaultWorkdir and
recreate the adapter runtime accordingly.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 594-611: In sendGeneratedImages, the catch block around
this.deps.client.sendImage currently calls this.deps.client.sendText directly
which can throw and break the for…of loop; wrap the fallback sendText call in
its own try/catch inside the same iteration so any failure to send the fallback
message is logged (e.g., with console.warn or processLogger) and does not abort
delivery of remaining assets—apply the same pattern to the analogous handlers in
weixinIlinkRuntime.ts and telegramPoller.ts.
In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts`:
- Around line 86-90: The early return on message.allAttachmentsFailed drops any
accompanying text; update the handler that checks message.allAttachmentsFailed
to preserve/forward message.text (or include it in the error reply) instead of
discarding it: when message.allAttachmentsFailed is true, construct the returned
object to include both replies: ['Failed to load your attachment. Please
resend.'] and also include the original message.text (e.g., append it to replies
or set a forwardedText field) so downstream logic (or the user-facing reply)
retains the user's text; locate the check for message.allAttachmentsFailed in
feishuCommandRouter.ts to implement this change.
In `@test/main/presenter/remoteControlPresenter/feishuAdapter.test.ts`:
- Around line 124-149: The test for FeishuAdapter.sendImage only asserts that
clientInstances[0].sendImage was called but doesn't verify the transport
payload; update the test to assert the adapter forwards the correct target
(parse 'oc_1:root' into chatId/threadId/replyToMessageId as appropriate) by
adding a toHaveBeenCalledWith(expect.objectContaining({ chatId: 'oc_1',
threadId: 'root', replyToMessageId: /* expected if any */ }),
'/tmp/generated.png') check so you ensure FeishuAdapter.sendImage builds and
passes the correct transport target to clientInstances[0].sendImage.
In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts`:
- Around line 93-111: The test calls parser.parseEvent without the botOpenId
causing inconsistency with other tests; update the call to include the bot open
id (e.g., 'ou_bot') so parseEvent(parser.parseEvent(...), or rather
parser.parseEvent(event, 'ou_bot')) is invoked with the same second-argument
signature used elsewhere, ensuring the parser receives the botOpenId when
processing the image message; locate the invocation of parseEvent in this test
(the parser.parseEvent(...) around the image message) and add the botOpenId
parameter.
In `@test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts`:
- Around line 128-284: Tests in remoteConversationRunner.test.ts create
temporary workspaces via fs.mkdtemp and never remove them; collect each mkdtemp
result (e.g., the workspace variables created in the two it blocks) into a
shared tempDirs array and add an afterEach(async () => { await
Promise.all(tempDirs.map(d => fs.rm(d, { recursive: true, force: true }))) }) to
remove them; ensure you push the workspace variable used to construct
preparedFile into tempDirs after creation and use await fs.rm with
recursive:true and force:true so cleanup runs reliably between tests.
🪄 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: bf83672b-7e01-4d4f-b9b4-785b91add90d
📒 Files selected for processing (24)
src/main/presenter/remoteControlPresenter/adapters/feishu/FeishuAdapter.tssrc/main/presenter/remoteControlPresenter/discord/discordRuntime.tssrc/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/feishu/feishuParser.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotParser.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/telegram/telegramClient.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/remoteControlPresenter/types.tssrc/main/presenter/remoteControlPresenter/types/channel.tssrc/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkRuntime.tssrc/renderer/settings/components/RemoteSettings.vuetest/main/presenter/remoteControlPresenter/feishuAdapter.test.tstest/main/presenter/remoteControlPresenter/feishuClient.test.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/feishuParser.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/qqbotParser.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/main/presenter/remoteControlPresenter/qqbotParser.test.ts
- src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts
- src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts
- src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts
- src/main/presenter/remoteControlPresenter/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts (1)
298-312: Remove theas anycast and the fields that don't belong inim.message.reply.The reply branch in
sendImagediverges from the typed pattern used insendText. Two issues:
params.receive_id_typeanddata.receive_idare not part of theim.message.replycontract — that endpoint targets the parent viapath.message_idonly. Those fields belong toim.message.create. They're only accepted here because(this.sdk.im.message.reply as any)suppresses the type error.- The
as anycast hides future breakage if the SDK signature changes, and the inconsistency withsendTextmakes the code harder to maintain.Align with the
sendTextreply shape to restore typed SDK calls:♻️ Proposed refactor
const response = target.replyToMessageId - ? await (this.sdk.im.message.reply as any)({ + ? ((await this.sdk.im.message.reply({ path: { message_id: target.replyToMessageId }, - params: { - receive_id_type: 'chat_id' - }, data: { - receive_id: target.chatId, msg_type: 'image', content: imageContent, reply_in_thread: Boolean(target.threadId) } - }) + })) as FeishuMessageResponse) : await this.sdk.im.message.create({ params: { receive_id_type: 'chat_id' }, data: { receive_id: target.chatId, msg_type: 'image', content: imageContent } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts` around lines 298 - 312, In sendImage, remove the unsafe cast "(this.sdk.im.message.reply as any)" and the invalid fields "params.receive_id_type" and "data.receive_id" so the call to this.sdk.im.message.reply matches the typed shape used in sendText; when replying (target.replyToMessageId) call this.sdk.im.message.reply with only the path.message_id and a data object containing msg_type, content (imageContent) and reply_in_thread as appropriate (use target.threadId to set reply_in_thread), keeping the same typed contract as sendText and relying on the SDK types for safety.
🤖 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/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Around line 653-660: The loop in remoteConversationRunner.ts that writes
attachments (the for (...) over attachments, using sanitizeFileName,
readAttachmentData, fs.writeFile and prepareMessageFile) can produce colliding
localPath names when multiple attachments share the same filename; modify the
code to ensure the on-disk filename is unique by incorporating the loop index
(or a short unique suffix) into the fileName used for localPath (e.g., prefix or
append before the extension) while keeping MessageFile.name as the original
sanitized filename if you want user-facing names preserved; update the call to
prepareMessageFile to use the unique localPath and correct byteLength but keep
metadata/name derived from sanitizeFileName so two attachments with the same
original filename do not overwrite each other on disk.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 298-312: In sendImage, remove the unsafe cast
"(this.sdk.im.message.reply as any)" and the invalid fields
"params.receive_id_type" and "data.receive_id" so the call to
this.sdk.im.message.reply matches the typed shape used in sendText; when
replying (target.replyToMessageId) call this.sdk.im.message.reply with only the
path.message_id and a data object containing msg_type, content (imageContent)
and reply_in_thread as appropriate (use target.threadId to set reply_in_thread),
keeping the same typed contract as sendText and relying on the SDK types for
safety.
🪄 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: 9b547b75-8f08-4d51-a72e-72c8533485cf
📒 Files selected for processing (6)
src/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tstest/main/presenter/remoteControlPresenter/feishuClient.test.tstest/main/presenter/remoteControlPresenter/qqbotRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/main/presenter/remoteControlPresenter/feishuClient.test.ts
- src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts
- test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts (2)
53-53: No cleanup strategy for.deepchat/remote-assets.Both
prepareRemoteAttachmentsandpersistGeneratedImageswrite under<workspace>/.deepchat/remote-assets/<channel>/<endpointHash>/<messageId>but nothing ever reaps these directories. Over long-running remote deployments, every inbound attachment and every generated image accumulates forever inside the user’s project, which can be sizable for image-heavy channels (Telegram albums, QQBot media). Worth adding a retention/cleanup job (e.g. on session deletion or a TTL sweep on startup) or at least documenting the expected size growth and manual-cleanup expectation.Also applies to: 942-998
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts` at line 53, The project writes files under REMOTE_ASSET_ROOT ('.deepchat/remote-assets') via prepareRemoteAttachments and persistGeneratedImages but never reaps them; add a cleanup strategy: implement a cleanupRemoteAssets function and call it on session deletion and at startup as a TTL sweep (e.g., remove per-channel/<endpointHash>/<messageId> directories older than a configurable retention period), or expose a config option and CLI/manual cleanup helper; ensure prepareRemoteAttachments and persistGeneratedImages create assets with metadata/timestamps to support TTL deletion and add a short note in README about expected growth and how to run the cleanup.
136-147: Document the double-decoded-hex-in-base64 branch.The fallback at Lines 144–146 (base64 decoding to 32 bytes whose ASCII form is hex, then hex-decoding again) is non-obvious and will be confusing to future maintainers. A one-line comment explaining which upstream producer emits keys in that shape (and why the straight 16-byte base64 case above isn’t sufficient) would save the next reader a round trip through git blame.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts` around lines 136 - 147, Add a one-line clarifying comment immediately above the fallback branch that handles base64-decoded 32-byte values whose ASCII is hex (the block using Buffer.from(value, 'base64'), decoded.length === 32 and decoded.toString('ascii') checked against /^[0-9a-fA-F]{32}$/ then re-hex-decoded) explaining that this handles keys emitted by the upstream producer that double-encodes a 16-byte key as ASCII hex then base64 (name the specific producer or protocol if known, e.g., the legacy remote-control client) and why the 16-byte base64 case above does not cover that shape.
🤖 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/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Around line 391-395: The code currently calls prepareRemoteAttachments and may
end up sending an empty text turn when input.attachments existed but all were
filtered out; update remoteConversationRunner.ts so after calling
prepareRemoteAttachments you detect the case where input.attachments?.length > 0
&& files.length === 0 && input.text.trim() === '' and handle it explicitly:
either throw a descriptive Error (e.g. "All attachments failed
validation/download") so the caller/router can surface the failure, or set text
to a clear fallback message (preserve the original fallback prompt) instead of
'', then build messageInput and call deps.agentSessionPresenter.sendMessage as
before; reference prepareRemoteAttachments, input, files, text, messageInput and
deps.agentSessionPresenter.sendMessage when making the change.
---
Nitpick comments:
In
`@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Line 53: The project writes files under REMOTE_ASSET_ROOT
('.deepchat/remote-assets') via prepareRemoteAttachments and
persistGeneratedImages but never reaps them; add a cleanup strategy: implement a
cleanupRemoteAssets function and call it on session deletion and at startup as a
TTL sweep (e.g., remove per-channel/<endpointHash>/<messageId> directories older
than a configurable retention period), or expose a config option and CLI/manual
cleanup helper; ensure prepareRemoteAttachments and persistGeneratedImages
create assets with metadata/timestamps to support TTL deletion and add a short
note in README about expected growth and how to run the cleanup.
- Around line 136-147: Add a one-line clarifying comment immediately above the
fallback branch that handles base64-decoded 32-byte values whose ASCII is hex
(the block using Buffer.from(value, 'base64'), decoded.length === 32 and
decoded.toString('ascii') checked against /^[0-9a-fA-F]{32}$/ then
re-hex-decoded) explaining that this handles keys emitted by the upstream
producer that double-encodes a 16-byte key as ASCII hex then base64 (name the
specific producer or protocol if known, e.g., the legacy remote-control client)
and why the 16-byte base64 case above does not cover that shape.
🪄 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: 12683623-2b94-492c-9d21-da7d7bb0ebf3
📒 Files selected for processing (2)
src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.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
Testing
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation