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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a registry-driven multi-channel remote-control system (Discord, QQBot, Weixin iLink) with adapters, runtimes, clients, parsers, auth/command routers, ChannelManager, binding-store extensions, Feishu/Lark brand support, a hooks→hook-commands migration, renderer/i18n updates, many tests, and related docs and provider/registry updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Gateway as Discord Gateway (WS)
participant Runtime as DiscordRuntime
participant Parser as DiscordParser
participant Auth as DiscordAuthGuard
participant Router as DiscordCommandRouter
participant Runner as RemoteConversationRunner
participant Store as RemoteBindingStore
participant REST as DiscordClient (REST)
Gateway->>Runtime: dispatch MESSAGE_CREATE / INTERACTION_CREATE
Runtime->>Parser: parseDispatch(payload, botUserId)
Parser-->>Runtime: DiscordInboundMessage
Runtime->>Auth: ensureAuthorized(message)
Auth-->>Runtime: { ok / not-ok }
alt authorized
Runtime->>Router: handleMessage(message)
Router-->>Runtime: { replies, conversation? }
alt conversation returned
Runtime->>Runner: deliverConversation(conversation)
Runner-->>Runtime: execution snapshots
Runtime->>REST: create/update messages (throttled edits)
Runtime->>Store: persist delivery state
else replies only
Runtime->>REST: sendMessage/reply
end
else not authorized
Runtime->>REST: sendPairInstruction or silent return
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
resources/acp-registry/registry.json (1)
460-505:⚠️ Potential issue | 🟡 MinorVersion field and download URLs are inconsistent for junie.
The
versionfield is"1321.57.0"(line 460), but all archive URLs reference1321.57without the trailing.0(e.g.,releases/download/1321.57/junie-release-1321.57-macos-aarch64.zip).This inconsistency could cause confusion when tracking versions or lead to cache/validation issues if version matching is strict.
Proposed fix: align version field with release tag
{ "id": "junie", "name": "Junie", - "version": "1321.57.0", + "version": "1321.57", "description": "AI Coding Agent by JetBrains",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/acp-registry/registry.json` around lines 460 - 505, The version field "1321.57.0" is inconsistent with the release tag used in all archive URLs ("1321.57"); update the registry entry so the version field matches the release tag used in the binary archives (or vice‑versa) to keep them identical—specifically change the "version" value or the archive URL segments that include "junie-release-1321.57" and "releases/download/1321.57" so they consistently use the same version string (e.g., "1321.57" or "1321.57.0") across the "version" key and all archive URLs for darwin-aarch64, darwin-x86_64, linux-aarch64, linux-x86_64, and windows-x86_64.src/renderer/src/i18n/da-DK/settings.json (1)
1178-1226:⚠️ Potential issue | 🟡 MinorUntranslated Chinese strings in Danish locale file.
Several strings in the ACP section remain in Chinese instead of Danish:
registryCount: "个 Agent"registryEmpty,registryRefresh,registryRepair: Chinese textfilters.*: Chinese filter labelsenvOverrideTitle,envOverridePlaceholder: ChineseinstallState.*: Chinese installation statesThese should be translated to Danish for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/da-DK/settings.json` around lines 1178 - 1226, The Danish locale contains untranslated Chinese values — update the entries registryCount, registryEmpty, registryRefresh, registryRepair, the filters object (all, enabled, installed, attention), envOverrideTitle, envOverridePlaceholder, and installState (installed, installing, error, notInstalled) with proper Danish translations to match surrounding strings; locate these keys in the da-DK/settings.json (see symbols registryCount, registryEmpty, registryRefresh, registryRepair, filters, envOverrideTitle, envOverridePlaceholder, installState) and replace the Chinese text with accurate Danish equivalents so the ACP section is fully localized.
🟡 Minor comments (13)
src/main/presenter/agentSessionPresenter/messageManager.ts-36-40 (1)
36-40:⚠️ Potential issue | 🟡 MinorFail fast when
sessionManageris not configured.If
NewMessageManageris constructed withoutsessionManager, this path throwsSession not found,
which is misleading and makes misconfiguration harder to diagnose. Add an explicit guard/error for
missingsessionManagerbefore lookup.Suggested fix
private resolveAgentForSession(sessionId: string) { - const session = this.sessionManager?.get(sessionId) + if (!this.sessionManager) { + throw new Error('NewMessageManager requires sessionManager for session-scoped operations') + } + const session = this.sessionManager.get(sessionId) if (!session) { throw new Error(`Session not found: ${sessionId}`) } return this.agentRegistry.resolve(session.agentId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentSessionPresenter/messageManager.ts` around lines 36 - 40, resolveAgentForSession currently assumes this.sessionManager exists and throws a misleading "Session not found" when sessionManager is missing; add an explicit guard at the start of resolveAgentForSession that checks this.sessionManager and throws a clear error (e.g., "sessionManager not configured") before calling this.sessionManager.get(sessionId). Reference the resolveAgentForSession method and the sessionManager property (and consider where NewMessageManager is constructed) to ensure callers see a precise misconfiguration error rather than a missing-session error.test/main/presenter/skillSyncPresenter/security.test.ts-282-290 (1)
282-290:⚠️ Potential issue | 🟡 MinorAssert the fallback actually checks
parentPath.This setup returns success for the second
access()call regardless of which path is retried, so a regression wherecheckWritePermission()recheckstargetPathinstead of the parent directory would still pass.🧪 Tighten the assertion
vi.mocked(fs.promises.access) .mockRejectedValueOnce(new Error('ENOENT')) // file doesn't exist .mockResolvedValue(undefined) // parent is writable vi.mocked(fs.promises.stat).mockResolvedValue({ isDirectory: () => true } as fs.Stats) const result = await checkWritePermission(targetPath) expect(result).toBe(true) + expect(fs.promises.access).toHaveBeenNthCalledWith(1, targetPath, fs.constants.W_OK) + expect(fs.promises.access).toHaveBeenNthCalledWith(2, parentPath, fs.constants.W_OK)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/skillSyncPresenter/security.test.ts` around lines 282 - 290, The test currently mocks fs.promises.access twice but doesn't verify which path is rechecked, so update the test around checkWritePermission to assert the second access call uses the parent directory: keep the first mockRejectedValueOnce(new Error('ENOENT')) and the second mockResolvedValue(undefined), then add an assertion on vi.mocked(fs.promises.access) (or the mocked access function) to ensure the second call's first argument equals the computed parentPath (the value passed into checkWritePermission's parent-path fallback) rather than targetPath; reference checkWritePermission, targetPath, parentPath, and fs.promises.access to locate where to add the stricter assertion.test/main/presenter/remoteControlPresenter/channelPluginManifest.test.ts-30-40 (1)
30-40:⚠️ Potential issue | 🟡 MinorSplit the invalid-manifest cases.
This assertion mixes the bad
pluginIdand the unsafeentrypath. If one validation regresses and the other still throws, the suite will keep passing.🧪 Suggested coverage split
it('rejects invalid or unsafe plugin manifests', () => { expect(() => parseChannelPluginManifest({ schemaVersion: CHANNEL_PLUGIN_SCHEMA_VERSION, pluginId: 'Custom Plugin', apiVersion: CHANNEL_PLUGIN_API_VERSION, - entry: '../index.js', + entry: 'dist/index.js', types: 'dist/index.d.ts', channelType: 'custom-im' }) ).toThrow() + + expect(() => + parseChannelPluginManifest({ + schemaVersion: CHANNEL_PLUGIN_SCHEMA_VERSION, + pluginId: 'custom.im', + apiVersion: CHANNEL_PLUGIN_API_VERSION, + entry: '../index.js', + types: 'dist/index.d.ts', + channelType: 'custom-im' + }) + ).toThrow()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/channelPluginManifest.test.ts` around lines 30 - 40, The test currently combines two failures so a regression in one won't be caught; split the single assertion into two focused tests calling parseChannelPluginManifest: one that passes a manifest with an invalid pluginId (e.g., 'Custom Plugin') and asserts it throws, and a second that passes a manifest with a safe pluginId but an unsafe entry path (e.g., '../index.js') and asserts it throws; use the same constants CHANNEL_PLUGIN_SCHEMA_VERSION and CHANNEL_PLUGIN_API_VERSION and the function parseChannelPluginManifest to locate the cases to change.src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts-116-116 (1)
116-116:⚠️ Potential issue | 🟡 MinorMention channel defaults in the ACP workdir error.
This runner now resolves per-channel workdirs before falling back to the global default, so telling users to set only a global directory points them at the wrong fix for channel-specific setups.
✏️ Suggested wording
- throw new Error('ACP agent requires a workdir. Set a global default directory first.') + throw new Error( + 'ACP agent requires a workdir. Set a channel default directory or a global default directory first.' + )🤖 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 116, The thrown error in remoteConversationRunner.ts ("throw new Error('ACP agent requires a workdir. Set a global default directory first.')") is misleading now that the runner resolves per-channel workdirs; update the error message in the RemoteConversationRunner (the function/method that throws this Error) to mention that a workdir can be set per-channel or via a global default and point users to check per-channel configurations as well as the global setting (e.g., "ACP agent requires a workdir: set a per-channel workdir or a global default directory. Check channel-specific settings if you intended a channel override.").src/renderer/src/i18n/pt-BR/settings.json-1532-1532 (1)
1532-1532:⚠️ Potential issue | 🟡 MinorAdd Discord to the overview sentence.
The page now includes Discord-specific remote-control settings, but this top-level description still lists only Telegram, Feishu, QQBot, and WeChat iLink.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/pt-BR/settings.json` at line 1532, Update the "description" value in the pt-BR settings JSON to include Discord in the list of supported remote-control integrations; locate the "description" key (the string currently reading "Configure em um só lugar o controle remoto por Telegram, Feishu, QQBot e WeChat iLink.") and modify its text to also mention Discord so the overview matches the page content.src/renderer/src/i18n/fa-IR/settings.json-1532-1532 (1)
1532-1532:⚠️ Potential issue | 🟡 MinorInclude Discord in the summary copy.
This description now fronts a remote-settings page that also has a Discord section below. Omitting Discord makes the supported-channel list look incomplete in this locale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fa-IR/settings.json` at line 1532, Update the JSON value for the "description" string that currently reads "کنترل از راه دور Telegram، Feishu، QQBot و WeChat iLink را در یکجا پیکربندی کنید." to include Discord as a supported channel (e.g., append or insert "و Discord" or the Persian transliteration "و دیسکورد") so the summary lists Telegram, Feishu, QQBot, WeChat iLink and Discord; locate the "description" key with that exact string and modify its value accordingly.src/renderer/src/i18n/ko-KR/settings.json-1532-1532 (1)
1532-1532:⚠️ Potential issue | 🟡 MinorInclude Discord in the Korean remote-control description.
This summary string still lists only Telegram, Feishu, QQBot, and WeChat iLink, so Korean users will see stale capability text even though Discord is now a supported channel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ko-KR/settings.json` at line 1532, Update the Korean localization string for the remote-control summary in src/renderer/src/i18n/ko-KR/settings.json by editing the "description" value to include Discord (디스코드) alongside Telegram, Feishu, QQBot, and WeChat iLink so the text reflects the newly supported channel; locate the "description" key in that file and add "디스코드" in the appropriate position and grammatical form within the comma-separated list.src/main/presenter/remoteControlPresenter/services/qqbotCommandRouter.ts-455-460 (1)
455-460:⚠️ Potential issue | 🟡 MinorHandle untitled sessions in QQBot replies.
/newaccepts an optional title, so these helpers can emitundefined [id]for untitled sessions. That leaks straight into several user-facing replies.🔧 Proposed fix
private formatSessionLabel(session: Pick<SessionWithState, 'id' | 'title'>): string { - return `${session.title} [${session.id}]` + return `${session.title?.trim() || 'Untitled'} [${session.id}]` } private formatSessionLine(session: SessionWithState, index: number): string { - return `${index}. ${session.title} [${session.id}]` + return `${index}. ${this.formatSessionLabel(session)}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/qqbotCommandRouter.ts` around lines 455 - 460, The helpers formatSessionLabel and formatSessionLine can emit "undefined [id]" when session.title is missing; update both functions (formatSessionLabel(session: Pick<SessionWithState, 'id' | 'title'>) and formatSessionLine(session: SessionWithState, index: number)) to normalize a missing or empty title into a human-friendly placeholder like "(untitled)" (or similar) before interpolating, so user-facing QQBot replies never show "undefined". Ensure you apply the same normalization logic in both methods.src/renderer/src/i18n/ru-RU/settings.json-1532-1532 (1)
1532-1532:⚠️ Potential issue | 🟡 MinorDiscord missing from the description string.
Same as the Japanese locale -
remote.descriptionomits Discord from the list.📝 Proposed fix
- "description": "Здесь можно централизованно настроить удалённое управление через Telegram, Feishu, QQBot и WeChat iLink.", + "description": "Здесь можно централизованно настроить удалённое управление через Telegram, Feishu, QQBot, Discord и WeChat iLink.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` at line 1532, The translation key "remote.description" in ru-RU settings (string currently listing "Telegram, Feishu, QQBot и WeChat iLink") omits Discord; update the value for "remote.description" to include "Discord" in the list (matching other locales), e.g. add "Discord" alongside Telegram/Feishu/QQBot/WeChat iLink so the string accurately reflects all supported remote providers.src/renderer/src/i18n/ja-JP/settings.json-1532-1532 (1)
1532-1532:⚠️ Potential issue | 🟡 MinorDiscord missing from the description string.
The
remote.descriptionlists "Telegram、Feishu、QQBot、WeChat iLink" but omits Discord, which has its own section below. Consider adding Discord to the description for consistency.📝 Proposed fix
- "description": "Telegram、Feishu、QQBot、WeChat iLink のリモート操作をまとめて設定します。", + "description": "Telegram、Feishu、QQBot、Discord、WeChat iLink のリモート操作をまとめて設定します。",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ja-JP/settings.json` at line 1532, Update the i18n string for remote.description to include Discord alongside Telegram、Feishu、QQBot、WeChat iLink so the description matches the available sections; locate the "remote.description" entry in src/renderer/src/i18n/ja-JP/settings.json and edit its value to add "Discord" in the same comma-separated Japanese list (e.g., "Telegram、Discord、Feishu、QQBot、WeChat iLink") ensuring punctuation and encoding match the existing strings.src/main/presenter/remoteControlPresenter/discord/discordParser.ts-107-114 (1)
107-114:⚠️ Potential issue | 🟡 MinorAddress static analysis ReDoS warning by sanitizing input.
The static analysis tool flagged potential ReDoS vulnerability when constructing a regex from
botUserId. While Discord bot user IDs are typically numeric snowflakes, sanitizing the input provides defense-in-depth.🛡️ Proposed fix to sanitize botUserId
const removeLeadingBotMentions = (text: string, botUserId: string | null | undefined): string => { if (!botUserId) { return text.trim() } + const sanitizedId = botUserId.replace(/[^0-9]/g, '') + if (!sanitizedId) { + return text.trim() + } + - const mentionPattern = new RegExp(`^(?:<@!?${botUserId}>\\s*)+`, 'i') + const mentionPattern = new RegExp(`^(?:<@!?${sanitizedId}>\\s*)+`, 'i') return text.replace(mentionPattern, '').trim() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/discord/discordParser.ts` around lines 107 - 114, The removeLeadingBotMentions function builds a RegExp from botUserId causing a possible ReDoS; sanitize or validate botUserId before using it: ensure botUserId matches the expected pattern (e.g., only digits / "snowflake" format) and if valid escape any regex metacharacters (or reject/return text.trim() for invalid input) before constructing mentionPattern; update the mentionPattern creation in removeLeadingBotMentions so it uses the sanitized/validated value and preserves the existing replace().trim() behavior.src/main/presenter/remoteControlPresenter/qqbot/qqbotClient.ts-96-99 (1)
96-99:⚠️ Potential issue | 🟡 MinorToken with invalid
expires_inwill be immediately considered expired.If the API returns an invalid or missing
expires_invalue, the token is stored butaccessTokenExpiresAtis set toDate.now(), meaning the token will always be refreshed on the next call. This causes unnecessary API calls. Consider logging a warning or using a default TTL.🛡️ Proposed fix with default TTL fallback
+const QQBOT_DEFAULT_TOKEN_TTL_MS = 7200 * 1000 // 2 hours fallback // In getAccessToken: this.accessToken = accessToken - this.accessTokenExpiresAt = - Date.now() + (Number.isFinite(expiresIn) && expiresIn > 0 ? expiresIn * 1000 : 0) + const ttlMs = Number.isFinite(expiresIn) && expiresIn > 0 + ? expiresIn * 1000 + : QQBOT_DEFAULT_TOKEN_TTL_MS + this.accessTokenExpiresAt = Date.now() + ttlMs return accessToken🤖 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 96 - 99, The current assignment to accessTokenExpiresAt treats missing/invalid expiresIn as zero, causing immediate expiry; update the logic around accessToken and accessTokenExpiresAt (the variables assigned in the qqbot client where accessToken and accessTokenExpiresAt are set) to: validate expiresIn, if it is missing/invalid use a sensible default TTL constant (e.g., DEFAULT_TOKEN_TTL_MS) and log a warning via the existing logger about using the fallback, and only compute accessTokenExpiresAt as Date.now() + ttl when ttl > 0 so tokens aren't force-refreshed on every call.src/main/presenter/remoteControlPresenter/channelAdapter.ts-257-266 (1)
257-266:⚠️ Potential issue | 🟡 MinorFull download occurs before size check when Content-Length is missing.
When the
content-lengthheader is missing or invalid (Line 246-247), the code downloads the entire response body before checking size at Line 259. For large files withoutContent-Length, this could consume significant memory before rejection.Consider using a streaming approach with a size-limited accumulator to reject oversized files during download rather than after.
🛡️ Stream-based size limiting approach
private async fetchBinaryAttachment(url: string): Promise<{...} | null> { const response = await net.fetch(url) if (!response.ok) { /* ... */ } // Check declared size first const contentLength = response.headers.get('content-length') const declaredSize = /* ... */ if (Number.isFinite(declaredSize) && declaredSize > CHANNEL_MAX_FILE_SIZE_BYTES) { return null } // Stream with size limit const reader = response.body?.getReader() if (!reader) return null const chunks: Uint8Array[] = [] let totalSize = 0 while (true) { const { done, value } = await reader.read() if (done) break totalSize += value.length if (totalSize > CHANNEL_MAX_FILE_SIZE_BYTES) { reader.cancel() this.emitLog('warn', 'Skipped oversized channel attachment.', { url, limit: CHANNEL_MAX_FILE_SIZE_BYTES }) return null } chunks.push(value) } const buffer = Buffer.concat(chunks) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/channelAdapter.ts` around lines 257 - 266, The code currently calls response.arrayBuffer() (see fetchBinaryAttachment) and only checks buffer.byteLength against CHANNEL_MAX_FILE_SIZE_BYTES after full download; change fetchBinaryAttachment to first check response.headers.get('content-length') and reject if declared size exceeds CHANNEL_MAX_FILE_SIZE_BYTES, and otherwise read response.body via a ReadableStream reader (response.body.getReader()), accumulate Uint8Array chunks while tracking totalSize, cancel the reader, emit the existing warn log and return null as soon as totalSize > CHANNEL_MAX_FILE_SIZE_BYTES, then use Buffer.concat(chunks) to create the final Buffer when download completes; also handle missing reader/response.body and non-ok responses as before.
🧹 Nitpick comments (24)
src/shared/providerDeeplink.ts (1)
4-35: Consider reducing provider-list duplication to prevent drift.This allowlist now needs synchronized edits with provider defaults and manual playground lists. A single shared source (or generated test fixture) would reduce future mismatch risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/providerDeeplink.ts` around lines 4 - 35, The SUPPORTED_PROVIDER_INSTALL_CUSTOM_TYPES array is duplicated across multiple places causing drift; extract this allowlist into one shared exported symbol (e.g., export const SUPPORTED_PROVIDER_INSTALL_CUSTOM_TYPES in a single providers/constants module or a generated fixture) and update all consumers (places that currently copy the list such as provider defaults and manual playground lists) to import that single source instead of maintaining local copies; ensure the new module exports the same readonly tuple type so existing usages (type assertions/calls expecting SUPPORTED_PROVIDER_INSTALL_CUSTOM_TYPES) keep compiling.test/renderer/components/LinkNode.test.ts (1)
92-95: Prefer patchingopenExternalover reassigningwindow.apiThe object spread pattern works in the current test setup because
window.apihaswritable: true, but it relies on all properties being enumerable. If the setup or preload definition changes to include non-enumerable properties, they would be lost by the spread operator. UsingObject.defineProperty()is more explicit and resilient.Proposed safer mock patch
+ let originalOpenExternal: typeof window.api.openExternal | undefined + beforeEach(() => { - window.api = { - ...window.api, - openExternal: vi.fn().mockResolvedValue(undefined) - } + originalOpenExternal = window.api?.openExternal + Object.defineProperty(window.api, 'openExternal', { + value: vi.fn().mockResolvedValue(undefined), + writable: true, + configurable: true + }) }) afterEach(() => { + if (originalOpenExternal) { + Object.defineProperty(window.api, 'openExternal', { + value: originalOpenExternal, + writable: true, + configurable: true + }) + } else { + delete (window.api as { openExternal?: unknown }).openExternal + } document.body.innerHTML = '' })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/LinkNode.test.ts` around lines 92 - 95, The test reassigns window.api via an object spread which can drop non-enumerable properties; instead patch the existing API by defining or overriding the openExternal property explicitly. Locate the test setup where window.api is modified (the openExternal mock in LinkNode.test.ts) and replace the spread-assignment with an explicit property definition using Object.defineProperty(window, 'api', ...) or Object.defineProperty(window.api, 'openExternal', ...) so that openExternal is set to a vi.fn().mockResolvedValue(undefined) without losing any non-enumerable properties on window.api.test/main/presenter/mcpClient.test.ts (2)
118-132: Avoid mutatingRuntimeHelperinternals directly in tests.Line 118-Line 132 rely on internal fields via casting, which makes tests fragile to internal refactors. Prefer a dedicated test reset API on
RuntimeHelper(for exampleresetForTest()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/mcpClient.test.ts` around lines 118 - 132, Tests currently mutate RuntimeHelper internals (runtimesInitialized, nodeRuntimePath, uvRuntimePath) via a cast which is fragile; add a public test-only method on RuntimeHelper named resetForTest() that resets runtimesInitialized = false, nodeRuntimePath = null, uvRuntimePath = null and restores any fs.existsSync mocking state expectations, then update tests to call RuntimeHelper.getInstance().resetForTest() instead of casting and directly setting fields and keep using mockFsExistsSync for fs.existsSync behavior.
160-161: Tighten executable assertions to prevent false positives.
toContain('npx'|'node'|'npm')can pass with incorrect executables. Consider asserting normalized basename/suffix instead (e.g., exact binary name with optional platform extension).Also applies to: 178-179, 260-261, 275-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/mcpClient.test.ts` around lines 160 - 161, The assertions using toContain on processedCommand.command are too loose and can yield false positives; update each check (e.g., the assertions around processedCommand.command at the spots referenced and the other occurrences) to assert the executable's basename exactly (allowing optional platform extension) — for example, extract the basename of processedCommand.command (or test against a regex that matches ^(?:npx|npx\.exe)$, ^(?:node|node\.exe)$, or ^(?:npm|npm\.exe)$ as appropriate) and assert equality or full-match rather than using toContain; apply this change where processedCommand.command is asserted (lines shown in the comment and the other mentioned occurrences) so the tests validate the actual binary name instead of a substring.test/main/presenter/SyncPresenter.test.ts (1)
107-112: Return the transaction callback result from the SQLite mock.
better-sqlite3transaction wrappers forward the wrapped function's return value. Returningundefinedhere makes the mock diverge from production behavior and can hide regressions if this path starts relying on that result.♻️ Keep the mock aligned with the library contract
- transaction<TArgs extends unknown[]>(fn: (...args: TArgs) => void) { - return (...args: TArgs) => { - fn(...args) + transaction<TArgs extends unknown[], TResult>(fn: (...args: TArgs) => TResult) { + return (...args: TArgs): TResult => { + const result = fn(...args) this.flush() + return result } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/SyncPresenter.test.ts` around lines 107 - 112, The mock transaction function (transaction<TArgs extends unknown[]>) currently discards the wrapped function's return value; modify it to capture and return that result so the mock matches better-sqlite3's contract: call const result = fn(...args), then invoke this.flush(), and finally return result, preserving generic types and call signature of the transaction wrapper.test/renderer/stores/pageRouter.test.ts (1)
88-101: Consider reusingsetupStorefor the failure-path case.The failure test redefines the same Pinia mocking block (Line 95 onward). Folding that into
setupStorewith an override parameter would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/stores/pageRouter.test.ts` around lines 88 - 101, The test duplicates the Pinia mock and agentSessionPresenter for the failure path; refactor by extending the existing setupStore helper to accept an override (e.g., a param for mocked services) and reuse it in the "falls back to new thread when active session lookup fails" test: update setupStore to take an overrides object (allowing you to pass a failing agentSessionPresenter that has getActiveSession mocked to reject) and remove the duplicated vi.doMock/agentSessionPresenter setup in that test, then call setupStore({ agentSessionPresenter: { getActiveSession: vi.fn().mockRejectedValue(new Error('boom')) } }) to set up the failure scenario.src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts (1)
32-38: TightenresolveLarkDomainreturn type tostring.The function always returns a fallback URL, so
string | undefinedis unnecessarily loose and leaks optionality into callers.Proposed diff
-const resolveLarkDomain = (brand: FeishuBrand): string | undefined => { +const resolveLarkDomain = (brand: FeishuBrand): string => { if (brand === 'lark') { return ((Lark as any).Domain?.Lark as string | undefined) ?? 'https://open.larksuite.com' } return ((Lark as any).Domain?.Feishu as string | undefined) ?? 'https://open.feishu.cn' }🤖 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 32 - 38, The resolveLarkDomain function currently types its return as string | undefined but always returns a fallback URL, so tighten the signature to return string and remove the undefined branch; change the function declaration to const resolveLarkDomain = (brand: FeishuBrand): string => { ... } and keep the existing fallback expressions using ((Lark as any).Domain?.Lark as string) ?? 'https://open.larksuite.com' and ((Lark as any).Domain?.Feishu as string) ?? 'https://open.feishu.cn' (or cast to string where needed), and update any callers that assumed undefined to treat the result as non-optional.test/main/presenter/remoteControlPresenter/feishuClient.test.ts (1)
49-49: Move this suite underremoteControlPresenter/feishu/.It tests
@/presenter/remoteControlPresenter/feishu/feishuClient, so the new file should mirror that source subtree instead of living at the presenter root.As per coding guidelines, "Vitest test suites should mirror the source structure under
test/main/**andtest/renderer/**".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuClient.test.ts` at line 49, This test file is in the wrong test subtree; move the Vitest suite for FeishuClient into a test path that mirrors the source: relocate the file from test/main/presenter/remoteControlPresenter/ to test/main/presenter/remoteControlPresenter/feishu/ (so it lives alongside the source module for FeishuClient), update any imports if the relative path changes, and ensure the test filename and describe() hierarchy still reference FeishuClient so the suite structure mirrors "@/presenter/remoteControlPresenter/feishu/feishuClient".test/main/presenter/remoteControlPresenter/channelPluginManifest.test.ts (1)
2-7: Move this suite underremoteControlPresenter/types/.This targets
@/presenter/remoteControlPresenter/types/channel, so the new test file should mirror that source subtree.As per coding guidelines, "Vitest test suites should mirror the source structure under
test/main/**andtest/renderer/**".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/channelPluginManifest.test.ts` around lines 2 - 7, The test file is located in the wrong test subtree; move the suite to mirror the source module path so it sits under test/main/presenter/remoteControlPresenter/types/ (matching the source module '@/presenter/remoteControlPresenter/types/channel'). Update the file path and any import roots if necessary so the test still imports CHANNEL_PLUGIN_API_VERSION, CHANNEL_PLUGIN_SCHEMA_VERSION, isChannelPluginManifest, and parseChannelPluginManifest from '@/presenter/remoteControlPresenter/types/channel'; ensure the test filename remains channelPluginManifest.test.ts and run Vitest to confirm resolution.test/main/presenter/skillSyncPresenter/formatConverter.test.ts (1)
105-116: Assert the frontmatter delimiters in the Cursor serialization test.This now only proves that
name: my-skillappears somewhere in the output. A serializer that emits plain markdown instead of a YAML frontmatter block would still pass.🧪 Tighten the assertion
const result = converter.serializeToExternal(skill, 'cursor') + expect(result).toContain('---') expect(result).toContain('name: my-skill') expect(result).toContain('A test skill') expect(result).toContain('Do something useful.')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/skillSyncPresenter/formatConverter.test.ts` around lines 105 - 116, The test 'should serialize to Cursor format' only checks for content but not that it's inside YAML frontmatter; update the assertion(s) around the call to converter.serializeToExternal(skill, 'cursor') to also assert the YAML frontmatter delimiters are present (e.g., the output starts with '---' and contains the closing '---' delimiter) so that serializeToExternal produces a proper frontmatter block rather than plain markdown.src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts (1)
434-451: Centralize the endpoint-prefix workdir lookup.This hardcodes five channel prefixes in another place. The next channel rename/addition will silently fall back to the global directory unless this chain is updated too.
♻️ Refactor sketch
+ private readonly channelWorkdirResolvers = [ + ['telegram:', () => this.bindingStore.getTelegramDefaultWorkdir()], + ['feishu:', () => this.bindingStore.getFeishuDefaultWorkdir()], + ['qqbot:', () => this.bindingStore.getQQBotDefaultWorkdir()], + ['discord:', () => this.bindingStore.getDiscordDefaultWorkdir()], + ['weixin-ilink:', () => this.bindingStore.getWeixinIlinkDefaultWorkdir()] + ] as const + private async resolveDefaultWorkdirForAgent( endpointKey: string, agentId: string ): Promise<string | null> { if ((await this.deps.configPresenter.getAgentType(agentId)) !== 'acp') { return null } - const channelDefaultWorkdir = endpointKey.startsWith('telegram:') - ? this.bindingStore.getTelegramDefaultWorkdir() - : endpointKey.startsWith('feishu:') - ? this.bindingStore.getFeishuDefaultWorkdir() - : endpointKey.startsWith('qqbot:') - ? this.bindingStore.getQQBotDefaultWorkdir() - : endpointKey.startsWith('discord:') - ? this.bindingStore.getDiscordDefaultWorkdir() - : endpointKey.startsWith('weixin-ilink:') - ? this.bindingStore.getWeixinIlinkDefaultWorkdir() - : '' + const channelDefaultWorkdir = this.channelWorkdirResolvers.find(([prefix]) => + endpointKey.startsWith(prefix) + )?.[1]() const normalizedChannelDefaultWorkdir = channelDefaultWorkdir?.trim() if (normalizedChannelDefaultWorkdir) { return normalizedChannelDefaultWorkdir }🤖 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 434 - 451, This code hardcodes channel prefix checks in remoteConversationRunner.ts (the channelDefaultWorkdir logic) so new channels can be missed; refactor by moving the prefix→workdir resolution into a single, reusable place (e.g. add a method like bindingStore.getDefaultWorkdirForEndpoint(endpointKey) or a helper function used across presenters) and replace the current conditional chain with a single call to that new method, falling back to this.getGlobalDefaultWorkdir() when it returns empty; reference the existing bindingStore getters (getTelegramDefaultWorkdir, getFeishuDefaultWorkdir, getQQBotDefaultWorkdir, getDiscordDefaultWorkdir, getWeixinIlinkDefaultWorkdir), the channelDefaultWorkdir variable, and the getGlobalDefaultWorkdir() call when implementing the centralized lookup.src/main/presenter/remoteControlPresenter/discord/discordParser.ts (1)
60-70: Consider renamingTelegramCommandPayloadfor cross-channel use.The
parseCommandfunction returnsTelegramCommandPayload, but this parser is for Discord. Consider creating a sharedRemoteCommandPayloadtype or renaming to avoid confusion, since the same structure is reused across channels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/discord/discordParser.ts` around lines 60 - 70, The parseCommand function in discordParser.ts returns TelegramCommandPayload which is misleading for a Discord parser; create a shared type (e.g., RemoteCommandPayload) that matches the existing shape (name: string, args: string) and replace usages of TelegramCommandPayload with this new type in parseCommand and any other channel parsers/consumers, update exports/imports accordingly, and ensure parseCommand's signature and return value are updated to use RemoteCommandPayload (or rename TelegramCommandPayload to a generic name if preferred) so the type reflects cross-channel reuse.src/main/presenter/remoteControlPresenter/services/discordCommandRouter.ts (1)
10-10: Consider renamingbuildFeishuPendingInteractionTextfor cross-channel use.The function
buildFeishuPendingInteractionTextis imported from Feishu-specific module but used in Discord routing. Since this is shared across channels (Discord, QQBot, WeixinIlink), consider renaming tobuildRemotePendingInteractionTextand moving to a shared location for clarity.Also applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/discordCommandRouter.ts` at line 10, The Feishu-specific function buildFeishuPendingInteractionText is being used by multiple channel routers (Discord, QQBot, WeixinIlink); rename it to buildRemotePendingInteractionText and relocate it from the feishu module into a shared presenter utility (e.g., presenter/remoteControlPresenter/shared or utils) so it's clearly cross-channel; update the export name in the new module and replace imports/usages in discordCommandRouter.ts and the QQBot/WeixinIlink files to import buildRemotePendingInteractionText from the shared location, and run tests/compile to ensure no remaining references to buildFeishuPendingInteractionText remain.src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts (2)
3-4: Cross-channel constants have channel-specific names.Constants
FEISHU_CONVERSATION_POLL_TIMEOUT_MSandTELEGRAM_STREAM_POLL_INTERVAL_MSare reused across Discord runtime but have channel-specific prefixes. Consider renaming toREMOTE_CONVERSATION_POLL_TIMEOUT_MSandREMOTE_STREAM_POLL_INTERVAL_MSfor clarity when used across channels.Also applies to: 484-484, 509-509
🤖 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 3 - 4, Rename the channel-specific constants FEISHU_CONVERSATION_POLL_TIMEOUT_MS and TELEGRAM_STREAM_POLL_INTERVAL_MS to generic names REMOTE_CONVERSATION_POLL_TIMEOUT_MS and REMOTE_STREAM_POLL_INTERVAL_MS, update their exports where they are defined, and replace all imports/usages in discordRuntime.ts (including the other occurrences of FEISHU_CONVERSATION_POLL_TIMEOUT_MS and TELEGRAM_STREAM_POLL_INTERVAL_MS) to use the new names (ensure you update any related documentation/comments). Also search the codebase for the old constant symbols and update those references to the new REMOTE_* names to keep naming consistent across channels.
118-118: SharedlastEditAtmay cause cross-endpoint throttling.The
lastEditAtfield is instance-level, meaning edits to different Discord channels/endpoints will throttle each other. If this is intentional (global rate limit), the current implementation is fine. If per-endpoint throttling is desired, consider trackinglastEditAtper endpoint key.♻️ Optional: Per-endpoint throttling
export class DiscordRuntime { private runId = 0 private started = false private stopRequested = false private readonly gateway: DiscordGatewaySession private statusSnapshot: DiscordRuntimeStatusSnapshot = { ... } private readonly processedInboundByMessage = new Map<string, DiscordProcessedInboundEntry>() private readonly endpointOperations = new Map<string, Promise<void>>() private applicationId: string | null = null - private lastEditAt = 0 + private readonly lastEditAtByEndpoint = new Map<string, number>() // In throttleEdit, accept endpointKey parameter: - private async throttleEdit(): Promise<void> { + private async throttleEdit(endpointKey: string): Promise<void> { const now = Date.now() - const elapsed = now - this.lastEditAt + const lastEditAt = this.lastEditAtByEndpoint.get(endpointKey) ?? 0 + const elapsed = now - lastEditAt if (elapsed < DISCORD_STREAM_EDIT_INTERVAL_MS) { await sleep(DISCORD_STREAM_EDIT_INTERVAL_MS - elapsed) } - this.lastEditAt = Date.now() + this.lastEditAtByEndpoint.set(endpointKey, Date.now()) }Also applies to: 772-779
🤖 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` at line 118, The field lastEditAt is shared across the DiscordRuntime instance and therefore causes edits on different endpoints/channels to throttle each other; change lastEditAt from a single number to a Map<string, number> (e.g., lastEditAtByEndpoint = new Map()) and use a per-endpoint key (channelId or composed endpoint key) wherever lastEditAt is read/updated (replace references to lastEditAt with lastEditAtByEndpoint.get(key) / .set(key, now)); update all methods that currently reference lastEditAt (including the uses around lines 772-779) to compute the endpoint key and read/update the map so throttling is applied per endpoint instead of globally.src/main/presenter/remoteControlPresenter/channelManager.ts (1)
146-154: Consider error handling strategy for connectAll/disconnectAll.
Promise.allwill reject on the first adapter failure, potentially leaving other adapters in inconsistent states. Consider whetherPromise.allSettledmight be more appropriate to ensure all adapters attempt their operation, with failures collected and reported together.♻️ Suggested alternative using allSettled
async connectAll(): Promise<void> { - await Promise.all([...this.adapters.values()].map(async (adapter) => await adapter.connect())) + const results = await Promise.allSettled( + [...this.adapters.values()].map(async (adapter) => await adapter.connect()) + ) + const failures = results.filter((r) => r.status === 'rejected') + if (failures.length > 0) { + throw new AggregateError( + failures.map((f) => (f as PromiseRejectedResult).reason), + `${failures.length} adapter(s) failed to connect` + ) + } } async disconnectAll(): Promise<void> { - await Promise.all( - [...this.adapters.values()].map(async (adapter) => await adapter.disconnect()) - ) + await Promise.allSettled( + [...this.adapters.values()].map(async (adapter) => await adapter.disconnect()) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/channelManager.ts` around lines 146 - 154, connectAll and disconnectAll currently use Promise.all which aborts on the first adapter failure and can leave other adapters unattempted; change both methods (connectAll, disconnectAll on the class managing this.adapters) to use Promise.allSettled so every adapter.connect()/adapter.disconnect() is attempted, collect any rejected results, log or aggregate their errors and then either throw a combined Error (with details) or return a summary object so callers can see which adapters failed and why.src/main/presenter/remoteControlPresenter/discord/discordClient.ts (1)
204-230: Content-Type header set unconditionally on all requests.The
requestmethod setsContent-Type: application/jsonfor all requests includingGETandDELETE. While most APIs ignore this, it's technically incorrect for bodyless requests and some strict servers may reject it.♻️ Set Content-Type only when body is present
private async request( path: string, init: RequestInit, auth: 'bot' | 'none' = 'bot' ): Promise<Response> { const headers: Record<string, string> = { - 'Content-Type': 'application/json' + ...(init.body ? { 'Content-Type': 'application/json' } : {}) }🤖 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 204 - 230, The request method currently always adds 'Content-Type: application/json' which is incorrect for bodyless requests; update the private async request(path, init, auth) implementation so the headers object only includes 'Content-Type' when init.body is present (or when method indicates a body like POST/PUT/PATCH), preserving any user-supplied init.headers and not overwriting them; locate the headers construction in request, conditionally add Content-Type before performing fetch, and keep the existing error handling that throws DiscordApiRequestError with normalizeResponseError unchanged.src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts (1)
814-918: Repetitive pairing failure handling could be consolidated.The
recordPairCodeFailuremethod has nearly identical logic repeated four times for each pairable channel. Consider extracting a helper to reduce duplication.♻️ Consolidated approach
private updatePairingFailure( channel: PairableRemoteChannel, maxAttempts: number ): { attempts: number; exhausted: boolean } { let result = { attempts: 0, exhausted: false } const updater = (pairing: { failedAttempts: number; code: string | null; expiresAt: number | null }) => { const attempts = pairing.failedAttempts + 1 const exhausted = attempts >= maxAttempts result = { attempts, exhausted } return exhausted ? { code: null, expiresAt: null, failedAttempts: 0 } : { ...pairing, failedAttempts: attempts } } // Apply to appropriate channel config // ... return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts` around lines 814 - 918, The recordPairCodeFailure function duplicates the same pairing-failure logic for each channel; extract a helper (e.g. private updatePairingFailure or updater function used inside recordPairCodeFailure) that accepts maxAttempts and a pairing object updater callback and returns {attempts, exhausted}, then call that helper from the existing channel-specific updaters (updateTelegramConfig, updateFeishuConfig, updateQQBotConfig, updateDiscordConfig) to mutate the channel pairing state consistently; keep the helper signature compatible with the pairing shape (failedAttempts, code, expiresAt) and ensure exhausted resets code/expiresAt/failedAttempts to 0 while non-exhausted increments failedAttempts.src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkClient.ts (1)
97-115: Module-level mutable state requires lifecycle consideration.The
activeLoginsMap is module-level state that persists for the app lifetime. WhilepurgeExpiredLogins()is called periodically, if the app runs for extended periods with many login attempts, entries may accumulate between purge cycles.Consider whether this should be tied to the presenter lifecycle or documented as intentionally process-scoped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/weixinIlink/weixinIlinkClient.ts` around lines 97 - 115, The module-level Map activeLogins and its helper functions (isFreshLogin, purgeExpiredLogins) create process-scoped state that can grow between purge cycles; refactor to tie this state to the presenter's lifecycle by moving activeLogins and purgeExpiredLogins into the presenter class or an instance-scoped LoginStore that the presenter constructs and disposes, add start/stop hooks to register/clear the purge interval, and expose a clear/close method (or ensure the presenter calls purgeExpiredLogins on shutdown) so entries cannot accumulate indefinitely; keep WEIXIN_ILINK_LOGIN_TTL_MS and logic but make the Map instance-scoped and ensure any periodic timer is cleaned up when the presenter is stopped.src/main/presenter/remoteControlPresenter/index.ts (5)
1894-1894: Use logger instead of console.warn for consistent logging.Line 1894 uses
console.warnwhile the rest of the file uses the importedlogger. This inconsistency could affect log aggregation in production.♻️ Proposed fix for consistent logging
private async registerTelegramCommands(client: TelegramClient): Promise<void> { try { await client.setMyCommands([...TELEGRAM_REMOTE_COMMANDS]) } catch (error) { - console.warn('[RemoteControlPresenter] Failed to register Telegram commands:', error) + logger.warn('[RemoteControlPresenter] Failed to register Telegram commands.', { error }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/index.ts` at line 1894, Replace the direct console.warn call with the module-level logger to keep logging consistent: locate the console.warn('[RemoteControlPresenter] Failed to register Telegram commands:', error) call inside RemoteControlPresenter (or the surrounding function handling Telegram command registration) and change it to use the imported logger (e.g., logger.warn or logger.error as appropriate), passing the same message and the error object so the error details are preserved in centralized logs.
1758-1761: Consider URL validation in setWindowOpenHandler for defense-in-depth.The handler loads any URL that the login page attempts to open via
window.open. While the security settings are properly hardened, validating that the URL matches expected domains (e.g., the original login URL's origin) would provide additional protection if the remote login page were compromised.🛡️ Proposed fix to add URL origin validation
+ const loginOrigin = new URL(normalizedLoginUrl).origin + loginWindow.webContents.setWindowOpenHandler(({ url }) => { - void loginWindow.loadURL(url) + try { + const targetOrigin = new URL(url).origin + if (targetOrigin === loginOrigin) { + void loginWindow.loadURL(url) + } else { + logger.warn('[RemoteControlPresenter] Blocked cross-origin navigation in login window.', { + targetUrl: url, + expectedOrigin: loginOrigin + }) + } + } catch { + logger.warn('[RemoteControlPresenter] Invalid URL in login window navigation.', { url }) + } return { action: 'deny' } })🤖 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 1758 - 1761, The setWindowOpenHandler currently loads any URL requested by window.open; update the handler in loginWindow.webContents.setWindowOpenHandler to validate the URL's origin before calling loginWindow.loadURL: parse the incoming url, compare its origin to the expected login origin (e.g., the original login URL's origin or an allowlist), and only call loginWindow.loadURL(url) when it matches; otherwise deny and optionally log the rejected URL for auditing. Ensure you reference the same symbols (loginWindow.webContents.setWindowOpenHandler and loginWindow.loadURL) when implementing the origin check.
1034-1038: Consider logging adapter connection failures for debugging.The empty catch blocks (also at lines 1072-1076, 1107-1111, 1141-1145) rely solely on the adapter status snapshot to capture failures. While the comment explains this, it's inconsistent with
rebuildWeixinIlinkAccountRuntime(line 1229-1233) which logs the error. Adding debug-level logging would help troubleshoot connection issues without affecting production behavior.♻️ Proposed fix to add consistent logging
try { await adapter.connect() - } catch { - // The adapter status snapshot already captures the failure. + } catch (error) { + logger.debug('[RemoteControlPresenter] Failed to connect Telegram adapter.', { error }) + // The adapter status snapshot also captures the failure for status queries. }🤖 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 1034 - 1038, The empty catch around await adapter.connect() should log the caught error at debug level to aid troubleshooting while preserving behavior; update the catch blocks (the one around adapter.connect() and the other empty catches near the same pattern) to catch the error (e) and call the module logger.debug or similar with a concise message and the error object—follow the pattern used in rebuildWeixinIlinkAccountRuntime to keep logging consistent and reference adapter.connect and the adapter status snapshot when composing the message.
1811-1821: Deeply nested ternaries reduce readability.The nested ternary operators for channel dispatch are harder to read and maintain than a switch statement or object lookup, especially as more channels are added.
♻️ Proposed refactor using object lookup
private getDefaultAgentId(channel: RemoteChannel): string { - return channel === 'telegram' - ? this.bindingStore.getTelegramDefaultAgentId() - : channel === 'feishu' - ? this.bindingStore.getFeishuDefaultAgentId() - : channel === 'qqbot' - ? this.bindingStore.getQQBotDefaultAgentId() - : channel === 'discord' - ? this.bindingStore.getDiscordDefaultAgentId() - : this.bindingStore.getWeixinIlinkDefaultAgentId() + const handlers: Record<RemoteChannel, () => string> = { + telegram: () => this.bindingStore.getTelegramDefaultAgentId(), + feishu: () => this.bindingStore.getFeishuDefaultAgentId(), + qqbot: () => this.bindingStore.getQQBotDefaultAgentId(), + discord: () => this.bindingStore.getDiscordDefaultAgentId(), + 'weixin-ilink': () => this.bindingStore.getWeixinIlinkDefaultAgentId() + } + return handlers[channel]() }🤖 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 1811 - 1821, The getDefaultAgentId function uses deeply nested ternary expressions; replace them with a clearer dispatch (e.g., a switch or an object map) that maps RemoteChannel values to the corresponding bindingStore accessors (getTelegramDefaultAgentId, getFeishuDefaultAgentId, getQQBotDefaultAgentId, getDiscordDefaultAgentId, getWeixinIlinkDefaultAgentId). Implement a lookup like const map = { telegram: this.bindingStore.getTelegramDefaultAgentId.bind(this.bindingStore), ... } and return map[channel]() (or use a switch that returns the same methods), ensuring you preserve existing behavior and return type in getDefaultAgentId.
364-391: Consider exhaustive channel handling for future maintainability.The method falls through to Discord handling on line 390 without an explicit channel check. If a new pairable channel is added, this could silently route to the wrong handler. A switch statement with exhaustive checking or an explicit
channel === 'discord'check would be safer.♻️ Proposed refactor for explicit handling
if (channel === 'qqbot') { this.bindingStore.removeQQBotPairedUser(normalizedPrincipalId) return } - this.bindingStore.removeDiscordPairedChannel(normalizedPrincipalId) + if (channel === 'discord') { + this.bindingStore.removeDiscordPairedChannel(normalizedPrincipalId) + return + } + + // Exhaustive check - TypeScript will error if a new channel is added + const _exhaustive: never = channel + throw new Error(`Unhandled pairable channel: ${_exhaustive}`)🤖 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 364 - 391, The removeChannelPrincipal method currently falls through to Discord handling for any non-telegram/feishu/qqbot channel; update removeChannelPrincipal to explicitly handle channels (e.g., use a switch on channel or add an explicit if (channel === 'discord') branch) and add a default case that either throws or logs an error for unknown PairableRemoteChannel values; ensure you call bindingStore.removeDiscordPairedChannel only when channel is 'discord' and reference the existing method name removeChannelPrincipal and bindingStore methods (removeAllowedUser, removeFeishuPairedUser, removeQQBotPairedUser, removeDiscordPairedChannel) when making the changes.
src/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.ts
Show resolved
Hide resolved
src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts
Show resolved
Hide resolved
Comment on lines
+155
to
+160
| async stop(): Promise<void> { | ||
| this.stopRequested = true | ||
| this.cleanupHeartbeat() | ||
| this.ws?.close() | ||
| this.ws = null | ||
| await this.runPromise |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '125,205p' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.ts
printf '\n----\n'
sed -n '216,440p' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.ts | \
rg -n 'firstConnected|rejectFirstConnected|sleep\(|addEventListener\(|removeEventListener\('Repository: ThinkInAIXYZ/deepchat
Length of output: 2691
🏁 Script executed:
sed -n '200,270p' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 1746
🏁 Script executed:
sed -n '270,350p' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 2316
🏁 Script executed:
rg -n 'function sleep|const sleep|export.*sleep' src/ --type ts --type tsxRepository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
rg -A 3 'export.*sleep|function sleep|const sleep' src/ -t tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3678
🏁 Script executed:
sed -n '430,460p' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 772
🏁 Script executed:
rg -n 'signal\.addEventListener' src/main/presenter/remoteControlPresenter/discord/discordGatewaySession.ts -A 2Repository: ThinkInAIXYZ/deepchat
Length of output: 146
Cancellation handling can leave firstConnectedPromise unsettled and block shutdown.
If the session aborts before READY, firstConnectedPromise is never settled—the loop exits, cleanup runs without resolving or rejecting the promise, and callers awaiting start() hang indefinitely. Additionally, the reconnect backoff loop uses await sleep(delayMs) without signal awareness, so stop() can be delayed by the full backoff window while the timeout completes. Each reconnect attempt also adds an abort listener to the signal without cleanup (though they do have { once: true } and will fire when aborted).
Fixes needed:
- Reject
firstConnectedPromisewhen the loop exits due to abort or stop beforeREADY - Make the backoff sleep interruptible (e.g.,
await Promise.race([sleep(delayMs), signal abort handler])or similar)
Relevant code
// start() awaits firstConnectedPromise but it may never be settled if signal aborts early
return await this.firstConnectedPromise
// Loop exits without settling the promise if signal.aborted is true
while (!this.stopRequested && !signal?.aborted) {
try {
await this.connectOnce(signal)
attempt = 0
} catch (error) {
// ... error handling with settlement logic
}
}
// No settlement here; finally block in start() just nullifies the promise reference
// Backoff is not cancellable
await sleep(delayMs)
// Listener accumulation on each retry
signal.addEventListener('abort', () => {
ws.close()
settleReject(new DOMException('Aborted', 'AbortError'))
}, { once: true })
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/renderer/src/i18n/fa-IR/settings.json (1)
79-145:⚠️ Potential issue | 🟡 MinorTranslate the new
fa-IRentries before shipping this locale update.Several added strings are still English, so Persian users will get a mixed-language settings screen. The new hook descriptions and multiple Telegram/Discord overview/help strings are the most visible examples here.
Also applies to: 1543-1660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fa-IR/settings.json` around lines 79 - 145, The fa-IR locale file contains untranslated English strings under the notificationsHooks block (notably keys under "commands" like "description", "hint", "commandPlaceholder", "guideDescription", "deliveryPlaceholder", "stdinPreviewLabel", "placeholdersTitle", "examplesTitle"/"exampleNodeLabel"/"examplePythonLabel"/"examplePowerShellLabel", and entries under "test" such as "duration", "statusCode", "exitCode", "retryAfter", "stdout", "stderr"); update these keys to proper Persian translations consistent with surrounding phrasing, ensure placeholders like "{index}", "{ms}", and "{code}" remain intact, and mirror translations for the other affected range (lines referenced in review, e.g., similar keys 1543-1660) so the settings UI is fully localized.src/renderer/src/i18n/ja-JP/settings.json (1)
79-145:⚠️ Potential issue | 🟡 MinorTranslate the new
ja-JPentries before landing this locale update.Several added strings are still English, so the Japanese settings UI will be visibly mixed-language. The new hooks copy and multiple Telegram/Discord help texts are the clearest examples in this patch.
Also applies to: 1543-1660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ja-JP/settings.json` around lines 79 - 145, The review found untranslated English strings in the ja-JP locale under the notificationsHooks block; update the remaining keys (e.g., notificationsHooks.commands.deliveryPlaceholder, exampleNodeLabel, examplePythonLabel, examplePowerShellLabel, metadataOnly, placeholdersDescription and any other English text in notificationsHooks.commands and notificationsHooks.test) to proper Japanese translations, ensuring consistency with existing phrasing; also scan the rest of the ja-JP file for other English fragments (the comment mentions additional untranslated ranges) and translate them before merging.src/types/i18n.d.ts (1)
2281-2311:⚠️ Potential issue | 🟠 Major
remote.remoteControlis still missing keys already present in the locale files.The locale payloads included here already contain
title,streamMode*,generatePairCode,clearPairCode,pairDialogInstruction,clearBindings, andclearBindingsResultunderremote.remoteControl, but this interface still omits them. The augmentation no longer matches the actualsettings.jsonshape, so typed locale loading and typedt()coverage will drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/i18n.d.ts` around lines 2281 - 2311, The remoteControl interface in src/types/i18n.d.ts is missing several keys present in the locale payloads; update the remoteControl type (the object under "remoteControl") to add the missing string properties: title, all streamMode* entries (match the specific streamMode keys used in locale files), generatePairCode, clearPairCode, pairDialogInstruction, clearBindings, and clearBindingsResult so the TypeScript shape matches settings.json and locale payloads and typed t() usage remains correct.
🧹 Nitpick comments (2)
docs/specs/remote-discord-lark/spec.md (2)
9-12: Tighten repetitive user-story openings for readability.Lines 9–12 repeatedly start with “As a DeepChat desktop user…”. Consider varying sentence structure to reduce visual repetition in the spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-discord-lark/spec.md` around lines 9 - 12, The three user-story lines that each begin with the exact phrase "As a DeepChat desktop user" are repetitive; reword them so the first story keeps that opener and the following stories use varied lead-ins (e.g., "Keep", "Allow", "Display") or transform them into concise acceptance-criteria style sentences while preserving intent for the Discord bot token, remote-control config isolation, shared Feishu/Lark UI, and consistent appearance of integrations (Discord, Feishu/Lark, QQBot, Telegram, Weixin iLink); update the lines in spec.md accordingly (refer to the exact story lines in the diff) so readability improves without changing the meaning.
32-35: Non-goals list is clear; minor wording polish can improve flow.Lines 32–35 use repeated “No …” starters. Rephrasing one or two bullets would make the section read less mechanically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-discord-lark/spec.md` around lines 32 - 35, Edit the "Non-goals" bullet list to avoid repetitive "No ..." sentence starters by rephrasing one or two items for better flow; specifically update the bullets that currently read "No standalone `lark` remote channel entity.", "No custom Lark domain input.", "No Hooks migration for Discord remote control.", and "No redesign of the remote binding or session-runner architecture." — for example, change one to "Exclude a standalone `lark` remote channel entity" and another to "Do not introduce a custom Lark domain input," or use "We will not migrate Hooks for Discord remote control" to vary phrasing while keeping the same meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/hooks-notifications/spec.md`:
- Around line 104-111: Update the spec text to avoid presenting an incomplete
environment contract: either enumerate the full set of environment variables
injected by the implementation (mirror what
src/main/presenter/hooksNotifications/index.ts actually sets, including
DEEPCHAT_HOOK_TIME, DEEPCHAT_HOOK_IS_TEST, plus provider/model/tool fields and
any other DEEPCHAT_* keys), or clearly mark the listed variables as a partial
example and add a sentence pointing readers to the implementation for the
authoritative list; ensure references to the variables in the spec match the
exact names used in the index.ts file so the spec and code stay synchronized.
In `@docs/specs/telegram-remote-control/plan.md`:
- Around line 47-48: The plan currently documents a Telegram-only IPC contract
but the implementation introduces a multi-channel presenter surface; update the
plan to reflect per-channel settings/status/pairing flows for all channels
(Telegram, QQBot, Discord, Weixin iLink) instead of only Telegram, and ensure
the spec artifacts include a multi-channel presenter interface (the
remote-control presenter surface and its methods for reading/saving settings,
runtime status, bindings, pairing snapshot, pair code management, and clearing
bindings) plus explicit tasks and resolved [NEEDS CLARIFICATION] items; keep the
spec under docs/specs/<feature>/, list the expected methods that mirror the new
presenter type (the remote-control presenter API surface), and add concrete
per-channel variations and acceptance criteria so follow-up work implements the
correct multi-channel IPC contract.
In `@src/main/presenter/hooksNotifications/config.ts`:
- Around line 22-25: The current HooksNotificationsSchema causes a single
malformed hook to fail safeParse and the code then resets the whole config to {
hooks: [] }; instead, make the top-level schema accept an array of unknowns
(e.g., hooks: z.array(z.unknown()).optional()) or keep HookCommandItemSchema but
stop relying on schema-wide safeParse to validate every item, then after parsing
iterate over the raw hooks and run HookCommandItemSchema.safeParse on each
entry, collecting only the successful parses into the normalized array; update
the code that currently calls safeParse and unconditionally sets { hooks: [] }
on failure to instead use the per-item filtered result (or preserve existing
hooks when no valid items) so one bad element no longer wipes the entire config.
- Around line 58-67: normalizeHookItem currently uses Boolean(input?.enabled)
which treats non-boolean truthy values like "false" or 1 as enabled; change the
enabled assignment in normalizeHookItem to only be true when input?.enabled ===
true so only an actual boolean true activates a hook (update the enabled field
generation in normalizeHookItem to use strict equality check).
In `@src/main/presenter/remoteControlPresenter/types.ts`:
- Around line 1656-1662: The snapshot builder buildQQBotPairingSnapshot is
omitting pairedGroupIds from QQBotRemoteRuntimeConfig, so authorized QQ groups
disappear from the presenter; update buildQQBotPairingSnapshot to include
pairedGroupIds (e.g., add pairedGroupIds: [...settings.pairedGroupIds]) and
ensure the QQBotPairingSnapshot contract/type includes the pairedGroupIds field
to mirror the runtime config.
In `@src/renderer/settings/components/NotificationsHooksSettings.vue`:
- Around line 539-552: In runHookTest, set the testing flag and clear
testResults for the hook synchronously before calling await persistConfig() so
the hook is marked busy immediately and a fast double-click can't start a second
run; specifically, update testing.value and testResults.value (for the given
hookId) prior to invoking persistConfig() in the runHookTest function to
serialize execution.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Line 1545: Update the "description" JSON value that enumerates remote-control
channels to include "Discord" so it matches the newly added remote.discord
section; locate the "description" key in the settings.json translations (the
string currently mentioning "Telegram, Feishu, QQBot og WeChat iLink") and
insert "Discord" into the comma-separated list and ensure localization grammar
remains correct.
- Line 1473: The Danish locale file contains new English strings (e.g., the
value "Configure lifecycle hook commands.") that must be translated into Danish;
update the corresponding da-DK entries by replacing the English phrases with
their correct Danish translations for the lifecycle hooks and remote-control
flows (including the other added English entries referenced around the file),
ensuring the JSON keys remain unchanged so the UI renders fully in Danish.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Line 81: Replace the remaining English strings in the Hebrew locale file by
translating the literal values (e.g., change "Configure lifecycle hook
commands." to an appropriate Hebrew sentence) for the entries shown in this diff
and the other ranges mentioned (lines 1548-1551, 1580, 1653) so the he-IL
settings and remote-control flows are fully localized; ensure you only change
the string values (keep the same JSON keys), use proper UTF-8 Hebrew text,
preserve valid JSON syntax, and run the i18n/l10n checks or build to verify
there are no missing translations or formatting errors.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Line 81: Finish translating all remaining English strings in the ko-KR
settings.json (e.g., the "description" value currently "Configure lifecycle hook
commands.") and translate the newly added blocks that include the Discord
section so the locale is fully Korean; update the summary string that documents
features to explicitly mention "Discord" (preserve the key names such as
"description" and any summary/title keys) and ensure all strings referenced in
the newly added ranges are translated into natural Korean with the same JSON
keys and punctuation preserved.
- Around line 1621-1628: The Korean pairing dialog still uses Telegram-specific
copy in the shared keys pairDialogTitle and pairDialogDescription; update ko-KR
to either replace those shared keys with neutral, channel-agnostic wording or
add new per-channel keys (e.g., pairDialogTitleFeishu,
pairDialogDescriptionFeishu, pairDialogTitleDiscord,
pairDialogDescriptionDiscord, pairDialogTitleQQBot, pairDialogDescriptionQQBot)
and ensure the UI uses the corresponding per-channel keys to display
Feishu/Discord/QQBot/WeChat-specific text instead of the Telegram text;
reference the existing per-channel instruction keys pairDialogInstructionFeishu,
pairDialogInstructionDiscord, pairDialogInstructionQQBot for naming consistency.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Line 81: Translate the remaining English strings in the Russian locale file by
replacing the English values with proper Russian translations for the lifecycle
hook and remote-control related entries (e.g., the "description" value for the
lifecycle hook commands and the other English strings referenced around the same
area); locate the keys in src/renderer/src/i18n/ru-RU/settings.json that
correspond to the hook settings and remote-control flows (the "description" key
shown and the nearby entries referenced in the comment) and update their string
values to correct Russian text so the locale no longer contains mixed
English/Russian phrases.
In `@src/shared/types/presenters/remote-control.presenter.d.ts`:
- Around line 240-264: The overloads for saveChannelSettings/getChannelSettings
allow mismatched channel/settings because the final catch-all uses RemoteChannel
and RemoteChannelSettings; replace the overloads with a single generic signature
that ties the channel literal to its specific settings via a mapped type (e.g.
define a ChannelSettingsMap mapping RemoteChannel -> specific setting types) and
use generics like <T extends RemoteChannel>(channel: T, input:
ChannelSettingsMap[T]) : Promise<ChannelSettingsMap[T]> (and the equivalent for
getChannelSettings) so the compiler enforces the correlation between the channel
argument and the settings payload.
In `@src/types/i18n.d.ts`:
- Around line 1131-1153: The locale type now requires the top-level "sort" and
"filter" blocks (including nested keys
capabilityOptions.{vision,functionCall,reasoning,search} and
typeOptions.{chat,embedding,rerank,imageGeneration}) as part of
DefineLocaleMessage; update the Hebrew, Russian and Chinese locale payloads to
include these keys with appropriate translations so they match the new type
shape and avoid runtime/typing breaks. Locate the locale JSON files that
correspond to settings (they currently omit "sort" and "filter") and add the
full structure shown (status, name under "sort" and label, clearAll,
visibleCount, capabilities, types, capabilityOptions, typeOptions under
"filter") with translated strings for each key. Ensure keys exactly match the
identifiers sort, filter, capabilityOptions, typeOptions and nested keys to
satisfy DefineLocaleMessage typing. Finally run the typecheck/localized build to
verify no missing keys remain.
---
Outside diff comments:
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 79-145: The fa-IR locale file contains untranslated English
strings under the notificationsHooks block (notably keys under "commands" like
"description", "hint", "commandPlaceholder", "guideDescription",
"deliveryPlaceholder", "stdinPreviewLabel", "placeholdersTitle",
"examplesTitle"/"exampleNodeLabel"/"examplePythonLabel"/"examplePowerShellLabel",
and entries under "test" such as "duration", "statusCode", "exitCode",
"retryAfter", "stdout", "stderr"); update these keys to proper Persian
translations consistent with surrounding phrasing, ensure placeholders like
"{index}", "{ms}", and "{code}" remain intact, and mirror translations for the
other affected range (lines referenced in review, e.g., similar keys 1543-1660)
so the settings UI is fully localized.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 79-145: The review found untranslated English strings in the ja-JP
locale under the notificationsHooks block; update the remaining keys (e.g.,
notificationsHooks.commands.deliveryPlaceholder, exampleNodeLabel,
examplePythonLabel, examplePowerShellLabel, metadataOnly,
placeholdersDescription and any other English text in
notificationsHooks.commands and notificationsHooks.test) to proper Japanese
translations, ensuring consistency with existing phrasing; also scan the rest of
the ja-JP file for other English fragments (the comment mentions additional
untranslated ranges) and translate them before merging.
In `@src/types/i18n.d.ts`:
- Around line 2281-2311: The remoteControl interface in src/types/i18n.d.ts is
missing several keys present in the locale payloads; update the remoteControl
type (the object under "remoteControl") to add the missing string properties:
title, all streamMode* entries (match the specific streamMode keys used in
locale files), generatePairCode, clearPairCode, pairDialogInstruction,
clearBindings, and clearBindingsResult so the TypeScript shape matches
settings.json and locale payloads and typed t() usage remains correct.
---
Nitpick comments:
In `@docs/specs/remote-discord-lark/spec.md`:
- Around line 9-12: The three user-story lines that each begin with the exact
phrase "As a DeepChat desktop user" are repetitive; reword them so the first
story keeps that opener and the following stories use varied lead-ins (e.g.,
"Keep", "Allow", "Display") or transform them into concise acceptance-criteria
style sentences while preserving intent for the Discord bot token,
remote-control config isolation, shared Feishu/Lark UI, and consistent
appearance of integrations (Discord, Feishu/Lark, QQBot, Telegram, Weixin
iLink); update the lines in spec.md accordingly (refer to the exact story lines
in the diff) so readability improves without changing the meaning.
- Around line 32-35: Edit the "Non-goals" bullet list to avoid repetitive "No
..." sentence starters by rephrasing one or two items for better flow;
specifically update the bullets that currently read "No standalone `lark` remote
channel entity.", "No custom Lark domain input.", "No Hooks migration for
Discord remote control.", and "No redesign of the remote binding or
session-runner architecture." — for example, change one to "Exclude a standalone
`lark` remote channel entity" and another to "Do not introduce a custom Lark
domain input," or use "We will not migrate Hooks for Discord remote control" to
vary phrasing while keeping the same meaning.
🪄 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: 001c21b7-694b-4284-a06c-b354a725d68a
📒 Files selected for processing (35)
docs/specs/hooks-notifications/plan.mddocs/specs/hooks-notifications/spec.mddocs/specs/remote-discord-lark/plan.mddocs/specs/remote-discord-lark/spec.mddocs/specs/telegram-remote-control/plan.mdsrc/main/presenter/configPresenter/index.tssrc/main/presenter/hooksNotifications/config.tssrc/main/presenter/hooksNotifications/index.tssrc/main/presenter/index.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/interface.tssrc/main/presenter/remoteControlPresenter/types.tssrc/renderer/settings/components/NotificationsHooksSettings.vuesrc/renderer/settings/components/RemoteSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/modelStore.tssrc/shared/hooksNotifications.tssrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/remote-control.presenter.d.tssrc/types/i18n.d.tstest/main/presenter/hooksNotifications.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/renderer/components/RemoteSettings.test.ts
✅ Files skipped from review due to trivial changes (4)
- src/renderer/src/stores/modelStore.ts
- docs/specs/hooks-notifications/plan.md
- docs/specs/remote-discord-lark/plan.md
- src/renderer/src/i18n/pt-BR/settings.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/presenter/index.ts
- src/shared/types/presenters/index.d.ts
- src/main/presenter/remoteControlPresenter/interface.ts
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/zh-TW/settings.json
- src/main/presenter/remoteControlPresenter/index.ts
Comment on lines
+1131
to
+1153
| sort: { | ||
| status: string | ||
| name: string | ||
| } | ||
| filter: { | ||
| label: string | ||
| clearAll: string | ||
| visibleCount: string | ||
| capabilities: string | ||
| types: string | ||
| capabilityOptions: { | ||
| vision: string | ||
| functionCall: string | ||
| reasoning: string | ||
| search: string | ||
| } | ||
| typeOptions: { | ||
| chat: string | ||
| embedding: string | ||
| rerank: string | ||
| imageGeneration: string | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add sort and filter translations before making them required.
DefineLocaleMessage now requires these blocks, but the updated locale payloads in this PR (src/renderer/src/i18n/he-IL/settings.json, src/renderer/src/i18n/ru-RU/settings.json, and src/renderer/src/i18n/zh-HK/settings.json) still omit both. That will leave the new list controls untranslated in those locales, and it will also break any strict typing of the locale objects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/i18n.d.ts` around lines 1131 - 1153, The locale type now requires
the top-level "sort" and "filter" blocks (including nested keys
capabilityOptions.{vision,functionCall,reasoning,search} and
typeOptions.{chat,embedding,rerank,imageGeneration}) as part of
DefineLocaleMessage; update the Hebrew, Russian and Chinese locale payloads to
include these keys with appropriate translations so they match the new type
shape and avoid runtime/typing breaks. Locate the locale JSON files that
correspond to settings (they currently omit "sort" and "filter") and add the
full structure shown (status, name under "sort" and label, clearAll,
visibleCount, capabilities, types, capabilityOptions, typeOptions under
"filter") with translated strings for each key. Ensure keys exactly match the
identifiers sort, filter, capabilityOptions, typeOptions and nested keys to
satisfy DefineLocaleMessage typing. Finally run the typecheck/localized build to
verify no missing keys remain.
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/lib/runtimeHelper.ts (1)
140-154:⚠️ Potential issue | 🟠 MajorPreserve manual runtime overrides across initialization.
At Line 140 and Line 152, manual values can be overwritten by a later
initializeRuntimes()call if set before first initialization. This makes the new override API
order-dependent.💡 Suggested fix
export class RuntimeHelper { private static instance: RuntimeHelper | null = null private nodeRuntimePath: string | null = null private uvRuntimePath: string | null = null + private nodeRuntimePathOverridden: boolean = false + private uvRuntimePathOverridden: boolean = false private ripgrepRuntimePath: string | null = null private rtkRuntimePath: string | null = null private runtimesInitialized: boolean = false public initializeRuntimes(force: boolean = false): void { @@ if (force) { this.nodeRuntimePath = null this.uvRuntimePath = null + this.nodeRuntimePathOverridden = false + this.uvRuntimePathOverridden = false this.ripgrepRuntimePath = null this.rtkRuntimePath = null } @@ - if (process.platform === 'win32') { + if (!this.nodeRuntimePathOverridden && process.platform === 'win32') { const nodeExe = path.join(nodeRuntimePath, 'node.exe') @@ - } else { + } else if (!this.nodeRuntimePathOverridden) { const nodeBin = path.join(nodeRuntimePath, 'bin', 'node') @@ - if (process.platform === 'win32') { + if (!this.uvRuntimePathOverridden && process.platform === 'win32') { const uvExe = path.join(uvRuntimePath, 'uv.exe') @@ - } else { + } else if (!this.uvRuntimePathOverridden) { const uvBin = path.join(uvRuntimePath, 'uv') @@ public setNodeRuntimePath(value: string | null): void { this.nodeRuntimePath = value + this.nodeRuntimePathOverridden = true } public setUvRuntimePath(value: string | null): void { this.uvRuntimePath = value + this.uvRuntimePathOverridden = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/runtimeHelper.ts` around lines 140 - 154, The setters setNodeRuntimePath and setUvRuntimePath currently allow manual values to be clobbered by initializeRuntimes; modify the logic so manual overrides are preserved by either (a) recording a manual flag (e.g., nodeRuntimePathIsManual, uvRuntimePathIsManual) when setNodeRuntimePath/setUvRuntimePath are called, or (b) having initializeRuntimes check for an existing non-null nodeRuntimePath/uvRuntimePath and skip overwriting them; update initializeRuntimes to honor those flags or non-null checks so manual values survive initialization and remove any code paths that unconditionally overwrite those properties.
♻️ Duplicate comments (1)
src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts (1)
134-145:⚠️ Potential issue | 🟠 MajorReject Telegram transport targets with extra
:segments.
split(':')still accepts values like123:456:extraand silently truncates them to chat123/ thread456. That can send a reply to the wrong topic instead of failing fast.🔧 Proposed fix
private parseTransportTarget(chatId: string): TelegramTransportTarget { - const [chatIdPart, messageThreadIdPart] = chatId.split(':') + const parts = chatId.split(':') + if (parts.length > 2) { + throw new Error(`Invalid Telegram chat id "${chatId}".`) + } + + const [chatIdPart, messageThreadIdPart] = parts const normalizedChatIdPart = chatIdPart.trim() const normalizedMessageThreadIdPart = messageThreadIdPart?.trim() ?? '' if (!/^-?\d+$/.test(normalizedChatIdPart)) { throw new Error(`Invalid Telegram chat id "${chatId}".`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts` around lines 134 - 145, The parseTransportTarget function currently uses chatId.split(':') which silently accepts inputs like "123:456:extra"; update parseTransportTarget (TelegramAdapter.parseTransportTarget) to reject any chatId containing more than two ':'-separated segments by checking the result of split and throwing an Error for inputs with length > 2, then proceed to trim/validate the two expected parts (chatIdPart and optional messageThreadIdPart) as before so only exact "chat" or "chat:thread" forms are allowed.
🧹 Nitpick comments (1)
docs/specs/remote-multi-channel/spec.md (1)
24-27: Consider varying repeated bullet openings for readability.Several consecutive bullets begin with the same phrase (
As a...andNo ...). Rewording one or two bullets would improve scanability without changing meaning.Also applies to: 76-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-multi-channel/spec.md` around lines 24 - 27, The repeated bullet openings ("As a..." and "No ...") in the Remote multi-channel spec reduce readability; rephrase one or two bullets to vary their lead-ins while preserving meaning—for example change "As a maintainer, I can add future built-in channels..." to "Maintainers can add future built-in channels by registering descriptors and adapters…" and alter one of the "As a..." bullets (or one of the "No ..." bullets referenced around the later section) to a passive or actorless phrasing so the intent stays identical but scanning is easier; update the bullets containing the phrases "As a desktop user", "As a maintainer", "As a WeChat iLink user" (and the later repeated "No ..." bullets) in docs/specs/remote-multi-channel/spec.md accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/remote-multi-channel/spec.md`:
- Around line 1-87: The spec file "spec.md" for the remote-multi-channel feature
is missing the required companion artifacts; add two new files alongside spec.md
named "plan.md" and "tasks.md" that live in the same feature folder, populate
plan.md with a high-level implementation plan matching the Acceptance Criteria
and Official Constraints from spec.md (milestones, rollout steps,
QA/compatibility checks), and populate tasks.md with concrete, trackable tasks
(epics, tickets, owners, estimated effort) that map to items like implementing
QQBotAdapter, WeixinIlinkAdapter, ChannelManager registration,
RemoteBindingStore/RemoteConversationRunner/RemoteBlockRenderer persistence
changes, and resolving any [NEEDS CLARIFICATION] notes referenced in the spec;
ensure the artifacts follow the project's spec/plan/tasks format and reference
the same feature identifiers used in spec.md.
In
`@src/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.ts`:
- Around line 122-129: The status handling in handleStatusChange currently sets
connected true for 'starting' and 'backoff' states; change it so connected is
true only when snapshot.state === 'running'. Update the connected expression in
handleStatusChange (which updates this.discordStatus and calls this.setStatus)
to check exclusively for 'running' and leave other fields (like state) unchanged
so consumers don't see Discord as available during startup or reconnect backoff.
In
`@src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts`:
- Around line 121-131: handleStatusChange currently sets connected true for
'starting' and 'backoff' states causing consumers to see Telegram as available
prematurely; change the logic in TelegramAdapter.handleStatusChange so connected
is true only when snapshot.state === 'running' (leave snapshot copy assignment
this.telegramStatus = { ...snapshot } and the other fields passed to setStatus
intact), i.e., update the connected expression passed to setStatus to check for
'running' only.
---
Outside diff comments:
In `@src/main/lib/runtimeHelper.ts`:
- Around line 140-154: The setters setNodeRuntimePath and setUvRuntimePath
currently allow manual values to be clobbered by initializeRuntimes; modify the
logic so manual overrides are preserved by either (a) recording a manual flag
(e.g., nodeRuntimePathIsManual, uvRuntimePathIsManual) when
setNodeRuntimePath/setUvRuntimePath are called, or (b) having initializeRuntimes
check for an existing non-null nodeRuntimePath/uvRuntimePath and skip
overwriting them; update initializeRuntimes to honor those flags or non-null
checks so manual values survive initialization and remove any code paths that
unconditionally overwrite those properties.
---
Duplicate comments:
In
`@src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts`:
- Around line 134-145: The parseTransportTarget function currently uses
chatId.split(':') which silently accepts inputs like "123:456:extra"; update
parseTransportTarget (TelegramAdapter.parseTransportTarget) to reject any chatId
containing more than two ':'-separated segments by checking the result of split
and throwing an Error for inputs with length > 2, then proceed to trim/validate
the two expected parts (chatIdPart and optional messageThreadIdPart) as before
so only exact "chat" or "chat:thread" forms are allowed.
---
Nitpick comments:
In `@docs/specs/remote-multi-channel/spec.md`:
- Around line 24-27: The repeated bullet openings ("As a..." and "No ...") in
the Remote multi-channel spec reduce readability; rephrase one or two bullets to
vary their lead-ins while preserving meaning—for example change "As a
maintainer, I can add future built-in channels..." to "Maintainers can add
future built-in channels by registering descriptors and adapters…" and alter one
of the "As a..." bullets (or one of the "No ..." bullets referenced around the
later section) to a passive or actorless phrasing so the intent stays identical
but scanning is easier; update the bullets containing the phrases "As a desktop
user", "As a maintainer", "As a WeChat iLink user" (and the later repeated "No
..." bullets) in docs/specs/remote-multi-channel/spec.md 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: e8e50fbb-39ba-453c-850b-175ec951442b
📒 Files selected for processing (13)
docs/specs/remote-multi-channel/spec.mdsrc/main/lib/runtimeHelper.tssrc/main/presenter/mcpPresenter/mcpClient.tssrc/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotGatewaySession.tssrc/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tssrc/renderer/settings/components/NotificationsHooksSettings.vuetest/main/presenter/mcpClient.test.tstest/main/presenter/remoteControlPresenter/discordAdapter.test.tstest/main/presenter/remoteControlPresenter/qqbotGatewaySession.test.tstest/main/presenter/remoteControlPresenter/qqbotRuntime.test.tstest/main/presenter/remoteControlPresenter/telegramAdapter.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/settings/components/NotificationsHooksSettings.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- test/main/presenter/mcpClient.test.ts
- src/main/presenter/mcpPresenter/mcpClient.ts
- src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts
Comment on lines
+1
to
+87
| # Remote Multi-Channel Foundation | ||
|
|
||
| ## Summary | ||
|
|
||
| Extend the existing Remote settings and runtime from Telegram-only to a fixed two-channel model: Telegram and Feishu. Telegram keeps hook notifications, while Feishu adds remote control only. Both channels continue to bind one remote endpoint to one DeepChat session and reuse the existing detached-session flow in Electron main. | ||
| DeepChat remote control now has a built-in adapter framework and a registry-driven renderer contract. The shipped built-in channel surface now covers Telegram, Discord, Feishu/Lark, QQ Bot Open Platform, and WeChat iLink. This spec captures the shared multi-channel foundation plus the QQBot and WeChat iLink delivery, while the Discord-specific behavior is detailed separately in `remote-discord-lark`. | ||
|
|
||
| This iteration also standardizes Telegram and Feishu on a compact remote delivery model: one temporary status message per assistant turn, one streamed answer track (one logical message sequence, delivered in one or more physical chunks as needed) that carries only user-visible answer content, and separate actionable prompts for pending interactions. | ||
| The design basis for QQBot follows Tencent official documentation only: | ||
|
|
||
| - access token and HTTP auth | ||
| - WebSocket gateway and intents | ||
| - official `C2C_MESSAGE_CREATE` and `GROUP_AT_MESSAGE_CREATE` events | ||
| - official `/v2/users/{openid}/messages` and `/v2/groups/{group_openid}/messages` send APIs | ||
|
|
||
| The WeChat iLink design basis follows Tencent official package behavior only: | ||
|
|
||
| - fixed QR base URL and official QR login flow | ||
| - `get_bot_qrcode` / `get_qrcode_status` polling with redirect-host handling | ||
| - `getupdates` long polling | ||
| - `sendmessage`, `getconfig`, and `sendtyping` | ||
| - multi-account runtime separation from day one | ||
|
|
||
| ## User Stories | ||
|
|
||
| - As a desktop user, I can configure Telegram and Feishu remote control from one Remote page without mixing their credentials and rules together. | ||
| - As a Telegram user, I can continue using the existing private-chat pairing flow and hook notifications. | ||
| - As a Feishu user, I can pair in a bot DM, then continue the same DeepChat session from DM, group chat, or topic thread. | ||
| - As an admin of the desktop app, I can see per-channel runtime health and binding counts from a shared overview area. | ||
| - As a paired Feishu user, I can trigger a remote session in group/topic only when I explicitly `@bot`. | ||
| - As a Telegram or Feishu user, I no longer need to read intermediate reasoning/tool/search transcript spam while the assistant is still running. | ||
| - As a desktop user, I can configure Telegram, Discord, Feishu/Lark, QQBot, and WeChat iLink remote control from one Remote settings page. | ||
| - As a maintainer, I can add future built-in channels by registering descriptors and adapters instead of hardcoding presenter branches. | ||
| - As a maintainer, I can keep Telegram, Discord, and Feishu/Lark behavior aligned through the same adapter boundary while extending the channel set with QQBot and WeChat iLink. | ||
| - As a WeChat iLink user, I can connect an official bot account by QR login and manage multiple connected accounts from the built-in settings page. | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - The Remote page renders a shared overview plus separate Telegram and Feishu tabs. | ||
| - Telegram settings continue to support bot token, remote pairing, allowlist, default agent, hook settings, and hook test actions. | ||
| - Feishu settings support app credentials, pairing, paired user management, default agent selection, and binding management. | ||
| - Feishu runtime runs in Electron main via WebSocket event subscription and does not require a renderer window. | ||
| - Feishu endpoints are keyed by `chatId + optional threadId`, with topic/thread replies isolated from the group root conversation. | ||
| - Feishu authorization requires DM pairing first; in groups/topics, only paired users who `@bot` may send commands or plain text to the bound session. | ||
| - `/pair`, `/new`, `/sessions`, `/use`, `/stop`, `/status`, `/open`, and `/model` work for Feishu remote control. | ||
| - Telegram `/model` continues to use inline keyboard menus; Feishu `/model` uses text commands only. | ||
| - Telegram and Feishu remote conversations use one temporary status message plus one streamed answer track (one logical message sequence, delivered in one or more physical chunks as needed) for normal assistant turns. | ||
| - When streamed answer text exceeds platform limits, earlier chunks within that answer track remain fixed and only the latest tail chunk stays editable. | ||
| - Reasoning-only, tool-call-only, tool-result-only, search-only, and pending-action-only assistant states never emit standalone transcript messages for normal remote delivery. | ||
| - Existing local desktop chat behavior remains unchanged. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - Keep a fixed two-channel architecture. Do not introduce a generic plugin registry for remote channels. | ||
| - Telegram hook notifications remain under the shared Remote page; Feishu hook notifications are out of scope. | ||
| - Remote sessions continue to use the existing `RemoteConversationRunner` and detached session creation path. | ||
| - Feishu v1 supports DM, group, and topic/thread input; media upload and user-OAuth automation remain out of scope. | ||
| - Pending interaction cards/prompts remain separate from the compact status-message lifecycle and do not erase already-streamed answer text. | ||
| - `IRemoteControlPresenter` exposes `listRemoteChannels()` and generic per-channel settings / status methods that include `qqbot` and `weixin-ilink`. | ||
| - `RemoteChannelId` includes: | ||
| - `telegram` | ||
| - `discord` | ||
| - `feishu` | ||
| - `qqbot` | ||
| - `weixin-ilink` | ||
| - Renderer remote UI is registry-driven for: | ||
| - overview cards | ||
| - tab headers | ||
| - sidebar remote status aggregation | ||
| - Existing Telegram and Feishu runtime behavior remains unchanged, and the same multi-channel adapter surface also supports the shipped Discord runtime described in `remote-discord-lark`. | ||
| - A built-in `QQBotAdapter` exists and is registered through `ChannelManager`. | ||
| - A built-in `WeixinIlinkAdapter` exists and is registered through `ChannelManager`. | ||
| - QQBot uses official transport primitives only: | ||
| - `POST https://bots.qq.com/app/getAppAccessToken` | ||
| - `GET https://api.sgroup.qq.com/gateway` | ||
| - official WebSocket `identify` / `resume` / heartbeat flow | ||
| - official C2C and group message send endpoints | ||
| - QQBot first-release scope is: | ||
| - C2C direct messages | ||
| - group `@bot` messages | ||
| - text-only passive replies | ||
| - WeChat iLink first-release scope is: | ||
| - official QR login | ||
| - multi-account management in settings | ||
| - owner-account-only remote control | ||
| - text-only direct replies | ||
| - `RemoteBindingStore`, `RemoteConversationRunner`, and `RemoteBlockRenderer` stay the source of truth for bindings, sessions, and rendered delivery text. | ||
| - Remote settings persist QQBot data under `remoteControl.qqbot` without flattening everything into a generic `channels` map. | ||
| - Remote settings persist WeChat iLink data under `remoteControl.weixinIlink`, including per-account runtime credentials and bindings. | ||
|
|
||
| ## Official Constraints | ||
|
|
||
| - QQ official C2C `user_openid` and group `member_openid` are different identity spaces, so a direct-message-paired user cannot be inferred from a group event. | ||
| - Because of that constraint, QQBot group authorization is handled separately from C2C user pairing. | ||
| - This iteration stores: | ||
| - paired C2C user ids | ||
| - internally authorized group ids | ||
| - Group control only reacts to `GROUP_AT_MESSAGE_CREATE` and only after explicit group authorization with `/pair`. | ||
| - WeChat iLink login and runtime follow the official QR + long-poll flow only; no personal WeChat bridge or unofficial protocol is used. | ||
| - This first WeChat iLink delivery authorizes only the owner account returned by QR login; it does not implement a secondary allowlist UI yet. | ||
|
|
||
| ## Non-Goals | ||
|
|
||
| - A general remote channel SDK or third-party channel plugin system. | ||
| - Feishu user-OAuth flows, approval cards, or hook notifications. | ||
| - Rich Feishu card-based model switching. | ||
| - Telegram group chat support. | ||
| - No OneBot, go-cqhttp, unofficial QQ bridges, or personal-WeChat bridges. | ||
| - No Slack runtime in this iteration. | ||
| - No secondary WeChat collaborator allowlist or shared-account UI in this iteration. | ||
| - No third-party plugin execution or installation UI in this iteration. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| - Existing `remoteControl.telegram` store data stays valid and is normalized into the new dual-channel config. | ||
| - Existing Telegram hook settings remain valid and continue to be saved through the Remote page. | ||
| - New Feishu-specific state is additive under `remoteControl.feishu`. | ||
| - Existing Telegram and Feishu saved settings remain valid. | ||
| - New WeChat iLink saved settings live beside existing remote settings without changing Telegram / Feishu / QQBot schema shapes. | ||
| - Existing Telegram hook test API remains valid. | ||
| - Existing renderer callers that use Telegram-only compatibility methods continue to work. | ||
| - New generic remote presenter methods become the preferred path for renderer code. |
Contributor
There was a problem hiding this comment.
Please add companion plan and tasks artifacts for this new spec package.
This PR includes docs/specs/remote-multi-channel/spec.md, but the feature-spec package should also include matching plan/tasks artifacts under the same feature folder.
As per coding guidelines, "Create specification artifacts under docs/specs/<feature>/ containing spec, plan, and tasks for new features, and resolve [NEEDS CLARIFICATION] items before implementation".
🧰 Tools
🪛 LanguageTool
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ead of hardcoding presenter branches. - As a maintainer, I can keep Telegram, Disc...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~27-~27: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nnel set with QQBot and WeChat iLink. - As a WeChat iLink user, I can connect an o...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~78-~78: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...- No Slack runtime in this iteration. - No secondary WeChat collaborator allowlist...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~79-~79: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... shared-account UI in this iteration. - No third-party plugin execution or install...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/remote-multi-channel/spec.md` around lines 1 - 87, The spec file
"spec.md" for the remote-multi-channel feature is missing the required companion
artifacts; add two new files alongside spec.md named "plan.md" and "tasks.md"
that live in the same feature folder, populate plan.md with a high-level
implementation plan matching the Acceptance Criteria and Official Constraints
from spec.md (milestones, rollout steps, QA/compatibility checks), and populate
tasks.md with concrete, trackable tasks (epics, tickets, owners, estimated
effort) that map to items like implementing QQBotAdapter, WeixinIlinkAdapter,
ChannelManager registration,
RemoteBindingStore/RemoteConversationRunner/RemoteBlockRenderer persistence
changes, and resolving any [NEEDS CLARIFICATION] notes referenced in the spec;
ensure the artifacts follow the project's spec/plan/tasks format and reference
the same feature identifiers used in spec.md.
src/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.ts
Show resolved
Hide resolved
src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/settings/components/NotificationsHooksSettings.vue (2)
4-8:⚠️ Potential issue | 🟡 MinorStart in loading state to avoid the false error flash.
With
configinitialised tonullandloadConfig()only kicked off inonMounted(), the first render falls into the!configbranch and briefly showsrequestFailedbefore the fetch starts. InitialisingisLoadingtotrueavoids that self-correcting error banner.💡 Suggested fix
const config = ref<HooksNotificationsSettings | null>(null) -const isLoading = ref(false) +const isLoading = ref(true) const isSaving = ref(false)Also applies to: 368-370, 582-584
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/NotificationsHooksSettings.vue` around lines 4 - 8, The component currently initialises isLoading false and config null so the first render shows the error banner; set isLoading to true by default and keep the loadConfig() call in onMounted() so the UI starts in the loading state until the async fetch resolves; update all similar patterns (the other occurrences around the blocks referencing isLoading, config, loadConfig, onMounted at the noted locations) so they also initialise isLoading=true to prevent the false requestFailed flash.
456-485:⚠️ Potential issue | 🟠 MajorMake
await persistConfig()actually wait for the save.When a save is already running, Lines 460-463 set
pendingSave = trueand return immediately.runHookTest()then falls through totestHookCommand()at Lines 545-546 before the latest edits are stored. BecausepersistConfig()also swallows save failures, a click on Test right after editing can execute against stale config after only showing the toast. This helper needs to expose an in-flight promise/result so callers can wait for the queued flush and abort on failure.Also applies to: 530-546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/NotificationsHooksSettings.vue` around lines 456 - 485, persistConfig currently returns immediately when a save is in-flight, so callers like runHookTest/testHookCommand can proceed against stale state; change persistConfig to track and return an in-flight Promise (e.g., add a local/shared inFlightSave Promise or savePromise variable) that represents the current or queued save, set pendingSave as before but instead of returning void create/return a Promise that waits for the next actual save (the call to configPresenter.setHooksNotificationsConfig) and rejects on error; ensure the try/catch/finally flow resolves or rejects that inFlight promise and clears it, and update callers (runHookTest/testHookCommand) to await persistConfig() so they block on the real save and can abort on failure.
♻️ Duplicate comments (2)
src/renderer/src/i18n/he-IL/settings.json (1)
1545-1545:⚠️ Potential issue | 🟡 MinorInclude Discord in the localized channel summary.
This summary lists Telegram/Feishu/QQBot/WeChat iLink but omits Discord while
remote.discordis defined a few lines below. Hebrew users won't see Discord advertised in the main description.Also applies to: 1575-1583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/he-IL/settings.json` at line 1545, The Hebrew localized summary string under the "description" key currently lists Telegram, Feishu, QQBot and WeChat iLink but omits Discord; update that "description" value(s) (around the same block as the remote.discord entry and also the strings at 1575-1583) to include Discord (e.g., add "Discord" in Hebrew/English as appropriate) so the localized channel summary advertises Discord alongside Telegram/Feishu/QQBot/WeChat iLink.src/renderer/src/i18n/ru-RU/settings.json (1)
1545-1545:⚠️ Potential issue | 🟡 MinorInclude Discord in the localized channel summary.
This overview sentence skips Discord even though the file defines a full
remote.discordblock right below it. Russian users will see the feature as unsupported from the main description.Also applies to: 1575-1583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` at line 1545, Update the localized summary string that lists supported remote channels to include Discord: edit the JSON value for the "description" key in the remote settings block (the localized string currently listing Telegram, Feishu, QQBot and WeChat iLink) to also mention Discord, and make the same change for the other summary occurrence referenced around lines 1575-1583 so the "remote.discord" feature is not omitted from the overview.
🤖 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/hooksNotifications/config.ts`:
- Around line 12-19: The HookCommandItemSchema currently requires events to be
an array of strings which causes safeParse to fail on mixed-type arrays and drop
the whole hook; change the schema's events entry to accept unknown (e.g.,
events: z.unknown().optional()) so validate/parsing won't reject malformed
arrays and then rely on sanitizeEvents() to filter and normalize valid event
names (update any other occurrence of this pattern noted near the hook
processing logic to match the same approach so sanitizeEvents() can handle
cleaning rather than the zod schema).
- Around line 103-115: The current reduce over rawHooks can leave duplicate
hook.id values, causing Vue key collisions; update the normalization step in the
hooks reduce so that before pushing each normalized item (where
normalizeHookItem(parsedItem.data, items.length) is called) you ensure the
returned HookCommandItem has a unique id not present in items (e.g., regenerate
or append a unique suffix/index when a conflict is detected). Modify the code
path around parsedItem/normalizeHookItem to detect existing ids in items and
produce a new unique id for the pushed item so keys used by the renderer
(hook.id) are deduplicated.
In
`@src/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.ts`:
- Around line 49-88: performConnect currently ignores the AbortSignal and on
failure only nulls references, which can leave DiscordRuntime/gateway running;
modify performConnect (and its startup sequence around runtime.start()) to:
listen to the provided _signal and if it becomes aborted call and await a proper
shutdown on the runtime (e.g., runtime.stop() or runtime.disconnect()) and tear
down the client, ensure the catch block not only nulls this.client/this.runtime
but also calls and awaits the runtime shutdown and client cleanup before
rethrowing the error, and also wire the abort handler to cancel the pending
runtime.start() promise so a partial startup cannot leave a running runtime;
reference performConnect, runtime.start(), and any runtime.stop()/disconnect()
or client cleanup methods to locate where to add these calls.
In `@src/main/presenter/remoteControlPresenter/types.ts`:
- Around line 1612-1619: The normalizer normalizeQQBotSettingsInput currently
only preserves pairedUserIds and drops pairedGroupIds; update this function to
also preserve pairedGroupIds by adding a pairedGroupIds property that mirrors
the user handling (e.g. pairedGroupIds:
normalizeQQBotGroupIds(input.pairedGroupIds) or, if normalizeQQBotGroupIds
doesn't exist, implement the same trimming/normalization used by
normalizeQQBotUserIds and call it here). Also ensure the shared settings
DTO/type for QQBotRemoteSettings includes pairedGroupIds so the value
round-trips through save paths.
---
Outside diff comments:
In `@src/renderer/settings/components/NotificationsHooksSettings.vue`:
- Around line 4-8: The component currently initialises isLoading false and
config null so the first render shows the error banner; set isLoading to true by
default and keep the loadConfig() call in onMounted() so the UI starts in the
loading state until the async fetch resolves; update all similar patterns (the
other occurrences around the blocks referencing isLoading, config, loadConfig,
onMounted at the noted locations) so they also initialise isLoading=true to
prevent the false requestFailed flash.
- Around line 456-485: persistConfig currently returns immediately when a save
is in-flight, so callers like runHookTest/testHookCommand can proceed against
stale state; change persistConfig to track and return an in-flight Promise
(e.g., add a local/shared inFlightSave Promise or savePromise variable) that
represents the current or queued save, set pendingSave as before but instead of
returning void create/return a Promise that waits for the next actual save (the
call to configPresenter.setHooksNotificationsConfig) and rejects on error;
ensure the try/catch/finally flow resolves or rejects that inFlight promise and
clears it, and update callers (runHookTest/testHookCommand) to await
persistConfig() so they block on the real save and can abort on failure.
---
Duplicate comments:
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Line 1545: The Hebrew localized summary string under the "description" key
currently lists Telegram, Feishu, QQBot and WeChat iLink but omits Discord;
update that "description" value(s) (around the same block as the remote.discord
entry and also the strings at 1575-1583) to include Discord (e.g., add "Discord"
in Hebrew/English as appropriate) so the localized channel summary advertises
Discord alongside Telegram/Feishu/QQBot/WeChat iLink.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Line 1545: Update the localized summary string that lists supported remote
channels to include Discord: edit the JSON value for the "description" key in
the remote settings block (the localized string currently listing Telegram,
Feishu, QQBot and WeChat iLink) to also mention Discord, and make the same
change for the other summary occurrence referenced around lines 1575-1583 so the
"remote.discord" feature is not omitted from the overview.
🪄 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: ba219f40-d96f-4e0e-a0dd-8444a2851a63
📒 Files selected for processing (22)
docs/specs/hooks-notifications/spec.mddocs/specs/remote-multi-channel/plan.mddocs/specs/remote-multi-channel/tasks.mddocs/specs/telegram-remote-control/plan.mddocs/specs/telegram-remote-control/spec.mddocs/specs/telegram-remote-control/tasks.mdsrc/main/presenter/hooksNotifications/config.tssrc/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.tssrc/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/types.tssrc/renderer/settings/components/NotificationsHooksSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/remote-control.presenter.d.tstest/main/presenter/hooksNotifications.test.tstest/main/presenter/remoteControlPresenter/discordAdapter.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/main/presenter/remoteControlPresenter/telegramAdapter.test.ts
✅ Files skipped from review due to trivial changes (3)
- docs/specs/telegram-remote-control/tasks.md
- docs/specs/hooks-notifications/spec.md
- src/main/presenter/remoteControlPresenter/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/specs/remote-multi-channel/tasks.md
- src/shared/types/presenters/index.d.ts
- src/main/presenter/remoteControlPresenter/adapters/telegram/TelegramAdapter.ts
- src/renderer/src/i18n/ko-KR/settings.json
Comment on lines
+12
to
19
| const HookCommandItemSchema = z | ||
| .object({ | ||
| enabled: z.boolean().optional(), | ||
| command: z.string().optional() | ||
| }) | ||
| .strip() | ||
|
|
||
| const HookCommandsSchema = z | ||
| .object({ | ||
| enabled: z.boolean().optional(), | ||
| events: z.record(z.string(), HookCommandConfigSchema).optional() | ||
| }) | ||
| .strip() | ||
|
|
||
| const TelegramSchema = z | ||
| .object({ | ||
| enabled: z.boolean().optional(), | ||
| botToken: z.string().optional(), | ||
| chatId: z.string().optional(), | ||
| threadId: z.union([z.string(), z.number()]).optional(), | ||
| events: z.array(z.string()).optional() | ||
| }) | ||
| .strip() | ||
|
|
||
| const DiscordSchema = z | ||
| .object({ | ||
| enabled: z.boolean().optional(), | ||
| webhookUrl: z.string().optional(), | ||
| events: z.array(z.string()).optional() | ||
| }) | ||
| .strip() | ||
|
|
||
| const ConfirmoSchema = z | ||
| .object({ | ||
| enabled: z.boolean().optional(), | ||
| id: z.unknown().optional(), | ||
| name: z.unknown().optional(), | ||
| enabled: z.unknown().optional(), | ||
| command: z.unknown().optional(), | ||
| events: z.array(z.string()).optional() | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't let a malformed events value drop the whole hook.
sanitizeEvents() already filters bad event names, but events: z.array(z.string()).optional() makes safeParse() fail first. A persisted hook like { events: ['SessionStart', 42] } now gets skipped at Lines 105-110 instead of surviving with the valid entry. Accept events as unknown input here and let sanitizeEvents() do the filtering.
💡 Suggested fix
const HookCommandItemSchema = z
.object({
id: z.unknown().optional(),
name: z.unknown().optional(),
enabled: z.unknown().optional(),
command: z.unknown().optional(),
- events: z.array(z.string()).optional()
+ events: z.unknown().optional()
})
.strip()
-const sanitizeEvents = (events?: string[] | null): HookEventName[] => {
+const sanitizeEvents = (events?: unknown): HookEventName[] => {
if (!Array.isArray(events)) {
return []
}
const sanitized = new Set<HookEventName>()
for (const eventName of events) {
- if (HOOK_EVENT_NAMES.includes(eventName as HookEventName)) {
+ if (typeof eventName === 'string' && HOOK_EVENT_NAMES.includes(eventName as HookEventName)) {
sanitized.add(eventName as HookEventName)
}
}
return Array.from(sanitized)
}Also applies to: 103-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/hooksNotifications/config.ts` around lines 12 - 19, The
HookCommandItemSchema currently requires events to be an array of strings which
causes safeParse to fail on mixed-type arrays and drop the whole hook; change
the schema's events entry to accept unknown (e.g., events:
z.unknown().optional()) so validate/parsing won't reject malformed arrays and
then rely on sanitizeEvents() to filter and normalize valid event names (update
any other occurrence of this pattern noted near the hook processing logic to
match the same approach so sanitizeEvents() can handle cleaning rather than the
zod schema).
Comment on lines
+103
to
+115
| const rawHooks = parsed.data.hooks ?? [] | ||
| const hooks = rawHooks.reduce<HookCommandItem[]>((items, item, index) => { | ||
| const parsedItem = HookCommandItemSchema.safeParse(item) | ||
| if (!parsedItem.success) { | ||
| log.warn( | ||
| `[HooksNotifications] Invalid hook at index ${index}, skipping: ${parsedItem.error?.message}` | ||
| ) | ||
| return items | ||
| } | ||
| } | ||
|
|
||
| const telegramEvents = sanitizeEvents(telegram.events) | ||
| const discordEvents = sanitizeEvents(discord.events) | ||
| const confirmoEvents = [...HOOK_EVENT_NAMES] | ||
|
|
||
| return { | ||
| telegram: { | ||
| ...defaults.telegram, | ||
| enabled: Boolean(telegram.enabled), | ||
| botToken: telegram.botToken ?? '', | ||
| chatId: telegram.chatId ?? '', | ||
| threadId: normalizeOptionalString(telegram.threadId), | ||
| events: telegramEvents.length ? telegramEvents : [...defaults.telegram.events] | ||
| }, | ||
| discord: { | ||
| ...defaults.discord, | ||
| enabled: Boolean(discord.enabled), | ||
| webhookUrl: discord.webhookUrl ?? '', | ||
| events: discordEvents.length ? discordEvents : [...defaults.discord.events] | ||
| }, | ||
| confirmo: { | ||
| ...defaults.confirmo, | ||
| enabled: Boolean(confirmo.enabled), | ||
| events: confirmoEvents | ||
| }, | ||
| commands: { | ||
| enabled: Boolean(commands.enabled), | ||
| events: normalizedCommandEvents | ||
| } | ||
| } | ||
| items.push(normalizeHookItem(parsedItem.data, items.length)) | ||
| return items | ||
| }, []) |
Contributor
There was a problem hiding this comment.
Regenerate duplicate hook ids during normalization.
The renderer uses hook.id as both the list key and the lookup key for toggle/remove/test operations in src/renderer/settings/components/NotificationsHooksSettings.vue. If synced or manually edited config contains duplicate ids, Vue keys collide, removeHook() drops every matching entry, and the per-hook test state overlaps. Deduplicate ids here before pushing normalized items.
💡 Suggested fix
const rawHooks = parsed.data.hooks ?? []
+ const seenIds = new Set<string>()
const hooks = rawHooks.reduce<HookCommandItem[]>((items, item, index) => {
const parsedItem = HookCommandItemSchema.safeParse(item)
if (!parsedItem.success) {
log.warn(
`[HooksNotifications] Invalid hook at index ${index}, skipping: ${parsedItem.error?.message}`
)
return items
}
- items.push(normalizeHookItem(parsedItem.data, items.length))
+ const normalized = normalizeHookItem(parsedItem.data, items.length)
+ while (seenIds.has(normalized.id)) {
+ normalized.id = randomUUID()
+ }
+ seenIds.add(normalized.id)
+ items.push(normalized)
return items
}, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/hooksNotifications/config.ts` around lines 103 - 115, The
current reduce over rawHooks can leave duplicate hook.id values, causing Vue key
collisions; update the normalization step in the hooks reduce so that before
pushing each normalized item (where normalizeHookItem(parsedItem.data,
items.length) is called) you ensure the returned HookCommandItem has a unique id
not present in items (e.g., regenerate or append a unique suffix/index when a
conflict is detected). Modify the code path around parsedItem/normalizeHookItem
to detect existing ids in items and produce a new unique id for the pushed item
so keys used by the renderer (hook.id) are deduplicated.
Comment on lines
+49
to
+88
| protected async performConnect(_signal: AbortSignal): Promise<void> { | ||
| if (!this.credentials.botToken) { | ||
| throw new Error('Bot token is required.') | ||
| } | ||
|
|
||
| const client = new DiscordClient(this.credentials) | ||
| const runtime = new DiscordRuntime({ | ||
| client, | ||
| parser: new DiscordParser(), | ||
| router: new DiscordCommandRouter({ | ||
| authGuard: new DiscordAuthGuard(this.bindingStore), | ||
| runner: this.createConversationRunner(), | ||
| bindingStore: this.bindingStore, | ||
| getRuntimeStatus: () => ({ ...this.discordStatus }) | ||
| }), | ||
| bindingStore: this.bindingStore, | ||
| logger, | ||
| onStatusChange: (snapshot) => { | ||
| this.handleStatusChange(snapshot) | ||
| }, | ||
| onFatalError: (message) => { | ||
| void this.onFatalError?.(message) | ||
| } | ||
| }) | ||
|
|
||
| this.client = client | ||
| this.runtime = runtime | ||
| this.handleStatusChange({ | ||
| state: 'starting', | ||
| lastError: null, | ||
| botUser: null | ||
| }) | ||
|
|
||
| try { | ||
| await runtime.start() | ||
| } catch (error) { | ||
| this.client = null | ||
| this.runtime = null | ||
| throw error | ||
| } |
Contributor
There was a problem hiding this comment.
Stop the runtime when startup is aborted or partially fails.
performConnect() ignores the AbortSignal, and the failure path only drops references. src/main/presenter/remoteControlPresenter/discord/discordRuntime.ts only marks startup as error; it does not tear the gateway back down. If startup fails after gateway.start() or disconnect() races with startup, this can leave a live runtime behind and let a stale connect win after teardown.
🔧 Suggested direction
- protected async performConnect(_signal: AbortSignal): Promise<void> {
+ protected async performConnect(signal: AbortSignal): Promise<void> {
if (!this.credentials.botToken) {
throw new Error('Bot token is required.')
}
const client = new DiscordClient(this.credentials)
const runtime = new DiscordRuntime({
client,
parser: new DiscordParser(),
router: new DiscordCommandRouter({
authGuard: new DiscordAuthGuard(this.bindingStore),
runner: this.createConversationRunner(),
bindingStore: this.bindingStore,
getRuntimeStatus: () => ({ ...this.discordStatus })
}),
bindingStore: this.bindingStore,
logger,
onStatusChange: (snapshot) => {
this.handleStatusChange(snapshot)
},
onFatalError: (message) => {
void this.onFatalError?.(message)
}
})
+ const abortListener = () => {
+ void runtime.stop().catch(() => undefined)
+ }
+
+ signal.addEventListener('abort', abortListener, { once: true })
this.client = client
this.runtime = runtime
this.handleStatusChange({
state: 'starting',
lastError: null,
botUser: null
})
try {
await runtime.start()
+ if (signal.aborted) {
+ await runtime.stop().catch(() => undefined)
+ throw new Error('Discord connection attempt was aborted.')
+ }
} catch (error) {
+ await runtime.stop().catch(() => undefined)
this.client = null
this.runtime = null
throw error
+ } finally {
+ signal.removeEventListener('abort', abortListener)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/adapters/discord/DiscordAdapter.ts`
around lines 49 - 88, performConnect currently ignores the AbortSignal and on
failure only nulls references, which can leave DiscordRuntime/gateway running;
modify performConnect (and its startup sequence around runtime.start()) to:
listen to the provided _signal and if it becomes aborted call and await a proper
shutdown on the runtime (e.g., runtime.stop() or runtime.disconnect()) and tear
down the client, ensure the catch block not only nulls this.client/this.runtime
but also calls and awaits the runtime shutdown and client cleanup before
rethrowing the error, and also wire the abort handler to cancel the pending
runtime.start() promise so a partial startup cannot leave a running runtime;
reference performConnect, runtime.start(), and any runtime.stop()/disconnect()
or client cleanup methods to locate where to add these calls.
Comment on lines
+1612
to
+1619
| export const normalizeQQBotSettingsInput = (input: QQBotRemoteSettings): QQBotRemoteSettings => ({ | ||
| appId: input.appId?.trim() ?? '', | ||
| clientSecret: input.clientSecret?.trim() ?? '', | ||
| remoteEnabled: Boolean(input.remoteEnabled), | ||
| defaultAgentId: input.defaultAgentId?.trim() || QQBOT_REMOTE_DEFAULT_AGENT_ID, | ||
| defaultWorkdir: input.defaultWorkdir?.trim() ?? '', | ||
| pairedUserIds: normalizeQQBotUserIds(input.pairedUserIds) | ||
| }) |
Contributor
There was a problem hiding this comment.
Preserve paired QQ groups in the settings normalizer.
This helper now round-trips pairedUserIds but not pairedGroupIds, even though the runtime config and pairing snapshot both carry both lists. Any save path that goes through this normalizer will silently drop paired groups. If the shared settings DTO still lacks pairedGroupIds, it needs to mirror the runtime change too.
🔧 Proposed fix
export const normalizeQQBotSettingsInput = (input: QQBotRemoteSettings): QQBotRemoteSettings => ({
appId: input.appId?.trim() ?? '',
clientSecret: input.clientSecret?.trim() ?? '',
remoteEnabled: Boolean(input.remoteEnabled),
defaultAgentId: input.defaultAgentId?.trim() || QQBOT_REMOTE_DEFAULT_AGENT_ID,
defaultWorkdir: input.defaultWorkdir?.trim() ?? '',
- pairedUserIds: normalizeQQBotUserIds(input.pairedUserIds)
+ pairedUserIds: normalizeQQBotUserIds(input.pairedUserIds),
+ pairedGroupIds: normalizeQQBotGroupIds(input.pairedGroupIds)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/types.ts` around lines 1612 - 1619,
The normalizer normalizeQQBotSettingsInput currently only preserves
pairedUserIds and drops pairedGroupIds; update this function to also preserve
pairedGroupIds by adding a pairedGroupIds property that mirrors the user
handling (e.g. pairedGroupIds: normalizeQQBotGroupIds(input.pairedGroupIds) or,
if normalizeQQBotGroupIds doesn't exist, implement the same
trimming/normalization used by normalizeQQBotUserIds and call it here). Also
ensure the shared settings DTO/type for QQBotRemoteSettings includes
pairedGroupIds so the value round-trips through save paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests