Conversation
📝 WalkthroughWalkthroughAdds Feishu as a second remote-control channel alongside Telegram: new Feishu client, parser, auth guard, command router, and runtime; refactors presenter, binding store, types, UI, and tests for channel-aware operation; adjusts Telegram draft-streaming and browser interactive-readiness logic. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,220,255,0.5)
participant Client as Renderer/Client
participant Presenter as RemoteControlPresenter
participant TgRuntime as TelegramRuntime
participant FeRuntime as FeishuRuntime
end
rect rgba(220,255,220,0.5)
participant FeClient as FeishuClient
participant FeRouter as FeishuCommandRouter
participant Runner as RemoteConversationRunner
end
Client->>Presenter: getChannelStatus(channel)
Presenter->>TgRuntime: getStatusSnapshot()
Presenter->>FeRuntime: getStatusSnapshot()
TgRuntime-->>Presenter: TelegramRemoteStatus
FeRuntime-->>Presenter: FeishuRemoteStatus
Presenter-->>Client: RemoteChannelStatus
Note over FeClient,FeRuntime: Feishu inbound flow
FeClient->>FeRuntime: onMessage(rawEvent)
FeRuntime->>FeRuntime: parse -> dedupe -> enqueue(endpoint)
FeRuntime->>FeRouter: handleMessage(parsed)
FeRouter->>FeRouter: authorize + route command
alt command triggers conversation
FeRouter->>Runner: create/send via RemoteConversationRunner
Runner-->>FeRuntime: execution snapshots (poll)
FeRuntime->>FeClient: sendText(streamed/final)
else simple reply
FeRouter-->>FeRuntime: reply text
FeRuntime->>FeClient: sendText(reply)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/renderer/src/i18n/he-IL/settings.json (1)
1498-1509:⚠️ Potential issue | 🟡 MinorThe shared pairing dialog copy is still Telegram-only.
This block adds Feishu-specific instruction text, but
pairDialogTitleandpairDialogDescriptionright above still hardcode Telegram. If the same dialog is reused for Feishu pairing, users will be pointed at the wrong bot/service. Split those strings per channel or make them neutral.🤖 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` around lines 1498 - 1509, The current translation keys pairDialogTitle and pairDialogDescription are Telegram-specific while you added pairDialogInstructionTelegram and pairDialogInstructionFeishu; either make the title/description neutral or add channel-specific keys (e.g., pairDialogTitleTelegram / pairDialogTitleFeishu and pairDialogDescriptionTelegram / pairDialogDescriptionFeishu) and update the dialog rendering to use the channel-specific keys based on the pairing flow; ensure callers that currently read pairDialogTitle/pairDialogDescription fall back to the neutral keys if channel-specific ones are missing.src/renderer/src/i18n/fa-IR/settings.json (1)
1498-1509:⚠️ Potential issue | 🟡 MinorFeishu dialog copy is still Telegram-specific in
fa-IR.The shared pairing/bindings flow is now channel-aware, but these strings still hard-code Telegram, so Persian users will see the wrong channel name in the Feishu flow. Please parameterize them by channel or add Feishu-specific variants.
🤖 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 1498 - 1509, The Persian strings mistakenly hard-code "Telegram" for channel-aware pairing; update the keys pairDialogInstruction, pairDialogInstructionTelegram and pairDialogInstructionFeishu in fa-IR settings.json so the Feishu flow shows the correct channel name: either parameterize pairDialogInstruction to accept a {channel} placeholder and translate it (e.g., "این فرمان را برای ربات {channel} بفرستید:"), or keep separate localized variants and ensure pairDialogInstructionFeishu uses Feishu wording (e.g., "این فرمان را برای ربات Feishu بفرستید:"); mirror the same approach for any binding/pairing keys that reference channels.src/renderer/src/i18n/ru-RU/settings.json (1)
1474-1509:⚠️ Potential issue | 🟡 MinorMake the RU pairing/bindings copy channel-aware.
pairDialogTitle,pairDialogDescription,bindingsDialogDescription, andbindingsEmptystill hard-code Telegram. The new Feishu flow reuses those keys, so Russian users will see the wrong provider text in Feishu dialogs.🤖 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` around lines 1474 - 1509, The RU strings for pairing/bindings are hard-coded to Telegram; make them channel-aware by adding separate keys for Telegram and Feishu (or replace the generic keys with channel-specific variants) and provide appropriate Russian copy for each. Concretely, add/replace keys such as pairDialogTitleTelegram / pairDialogTitleFeishu, pairDialogDescriptionTelegram / pairDialogDescriptionFeishu, bindingsDialogDescriptionTelegram / bindingsDialogDescriptionFeishu, and bindingsEmptyTelegram / bindingsEmptyFeishu (keeping existing pairDialogInstructionTelegram and pairDialogInstructionFeishu patterns), then update the consumer to use the channel-specific key based on the provider (e.g., when displaying pair dialog or bindings list, choose the Telegram or Feishu variant).src/renderer/settings/components/RemoteSettings.vue (1)
1082-1123:⚠️ Potential issue | 🟠 MajorLoad/status refresh should degrade per channel, not all-or-nothing.
refreshStatus()andloadState()both usePromise.all, so one Feishu fetch failure prevents Telegram from loading/updating at all even though the channels are independent. Keep the healthy channel usable and preserve last-known data for the failing one.🛠️ Example isolation for the status refresh path
const refreshStatus = async () => { - const [nextTelegramStatus, nextFeishuStatus] = await Promise.all([ + const [telegramResult, feishuResult] = await Promise.allSettled([ getChannelStatusCompat('telegram'), getChannelStatusCompat('feishu') ]) - telegramStatus.value = nextTelegramStatus - feishuStatus.value = nextFeishuStatus + if (telegramResult.status === 'fulfilled') { + telegramStatus.value = telegramResult.value + } + if (feishuResult.status === 'fulfilled') { + feishuStatus.value = feishuResult.value + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/RemoteSettings.vue` around lines 1082 - 1123, refreshStatus and loadState use Promise.all which makes one channel failure block the other; change these to fetch per-channel (either via Promise.allSettled or separate try/catch awaits) so you only update telegramStatus.value/feishuStatus.value when that channel's fetch succeeds and preserve the previous value on failure. In refreshStatus call getChannelStatusCompat('telegram') and getChannelStatusCompat('feishu') independently and update telegramStatus/feishuStatus only on success; in loadState await getChannelSettingsCompat/getChannelStatusCompat for each channel in isolated try/catch blocks (or use Promise.allSettled and check each result) and call syncTelegramFields/syncFeishuFields only with successful loaded settings; also ensure loadDeepChatAgents (newAgentPresenter.getAgents) is awaited separately so its failure doesn't block channel loads.
🧹 Nitpick comments (9)
test/renderer/components/WindowSideBar.test.ts (1)
310-339: Consider adding Feishu-specific test scenarios.The current tests only verify Telegram-enabled states. For complete coverage of the multi-channel feature, consider adding test cases where Feishu is enabled (either alone or alongside Telegram) to verify the combined remote-control status tooltip behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/WindowSideBar.test.ts` around lines 310 - 339, The test covers only Telegram remote-control states; add Feishu-specific scenarios by reusing the existing setup helper (call setup with remoteStatus plus channel flags indicating Feishu enabled, e.g., include a channels or integrations field enabling Feishu) and add two new assertions: one where Feishu alone is enabled (ensure wrapper.find('[data-testid="remote-control-button"]').exists() is true and the button classes/status text reflect Feishu) and one where both Telegram and Feishu are enabled (ensure the button exists and the tooltip/text contains the combined/Feishu-specific label), using the same selectors and checks as in the existing "shows the remote control button only when remote control is enabled" test to validate tooltip/label behavior when Feishu is present.test/main/presenter/remoteControlPresenter/feishuCommandRouter.test.ts (1)
1-2: Move this suite to the mirroredservices/path.This file covers
src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts, so the matching test location should betest/main/presenter/remoteControlPresenter/services/feishuCommandRouter.test.ts.Based on learnings, Applies to
test/**/*.{test,spec}.ts: Use Vitest with jsdom and Vue Test Utils for testing; locate test files mirroring source structure undertest/main/**andtest/renderer/**; name files as*.test.tsor*.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuCommandRouter.test.ts` around lines 1 - 2, The test suite for FeishuCommandRouter is in the wrong location; move this file so it mirrors the source module under the services path (i.e., place the test alongside the implementation at test/main/presenter/remoteControlPresenter/services/feishuCommandRouter.test.ts), ensuring the import of FeishuCommandRouter remains valid; confirm the test runner uses Vitest with jsdom and Vue Test Utils per project conventions and that the filename follows the *.test.ts pattern.src/main/presenter/remoteControlPresenter/services/remoteAuthGuard.ts (1)
65-69: Extract the compatclearPairCodedispatch once.The same arity branch appears in both clearing paths. A small private helper would keep the Telegram fallback behavior from drifting the next time this compat layer changes.
Also applies to: 79-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteAuthGuard.ts` around lines 65 - 69, The compat code repeats branching logic for dispatching bindingStoreCompat.clearPairCode; extract a small private helper (e.g., callClearPairCode or clearPairCodeCompat) that accepts no args and internally checks bindingStoreCompat.clearPairCode.length to either call bindingStoreCompat.clearPairCode() or bindingStoreCompat.clearPairCode('telegram'), then replace both existing branches (the block using bindingStoreCompat.clearPairCode at the two locations) with a single call to that helper so the Telegram fallback behavior is centralized and not duplicated.test/main/presenter/remoteControlPresenter/feishuParser.test.ts (1)
1-2: Mirror the source directory in the test path.This suite targets
src/main/presenter/remoteControlPresenter/feishu/feishuParser.ts, so the mirrored test location should betest/main/presenter/remoteControlPresenter/feishu/feishuParser.test.tsinstead of the flattenedremoteControlPresenter/root.Based on learnings, Applies to
test/**/*.{test,spec}.ts: Use Vitest with jsdom and Vue Test Utils for testing; locate test files mirroring source structure undertest/main/**andtest/renderer/**; name files as*.test.tsor*.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts` around lines 1 - 2, Test file is placed at a flattened path instead of mirroring the source directory; move the test so its relative directory structure matches the source (same nested folders as the module under test) and rename/locate it to follow the test naming convention (*.test.ts or *.spec.ts). Update the test harness to run with Vitest configured for jsdom and Vue Test Utils, and ensure the test imports the FeishuParser symbol from its module path as in the source so the test resolves the same module layout the source uses.docs/specs/remote-multi-channel/tasks.md (1)
8-11: Add typecheck and coverage to the final validation step.The checklist currently stops at lint + targeted Vitest suites, so it misses the repo’s documented typecheck and
pnpm run test:coveragegates for TS/test changes.As per coding guidelines,
**/*.{js,ts,tsx}: Use OxLint for JavaScript and TypeScript linting; ensure lint-staged hooks and typecheck pass before commits, andtest/**/*.ts: Runpnpm run test:coverageto verify test coverage during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-multi-channel/tasks.md` around lines 8 - 11, Checklist misses the repo typecheck and coverage gates—append typecheck and coverage steps to the final validation list. Update the list item that currently reads "Run `pnpm run format`, `pnpm run i18n`, `pnpm run lint`, and targeted Vitest suites" to also run the TypeScript typecheck and coverage commands (e.g., add `pnpm run typecheck` and `pnpm run test:coverage`), and note the patterns to enforce (`**/*.{js,ts,tsx}` via OxLint/lint-staged and `test/**/*.ts` for coverage) so contributors run both linters and the coverage/typecheck gates before completing the task.src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
269-274: Consider validating text before sending on completion.When
snapshot.completedis true, the code sendssnapshot.textwithout checking if it's empty. If the conversation completed without producing text, this might send an empty or placeholder message.♻️ Proposed improvement
if (snapshot.completed) { + if (snapshot.text.trim()) { await this.deps.client.sendText(target, snapshot.text) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts` around lines 269 - 274, The completion branch sends snapshot.text blindly when snapshot.completed is true; update the logic in the loop around execution.getSnapshot() to validate snapshot.text (e.g., check for null/undefined and trim().length > 0) before calling this.deps.client.sendText(target, snapshot.text); if the text is empty, skip sending (or log/notify via this.deps.logger) and then return—ensure you reference snapshot.completed, snapshot.text, execution.getSnapshot(), and this.deps.client.sendText(target, ...) when making the change.src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts (1)
116-124: Consider making logger level configurable.
LoggerLevel.infoon line 119 may produce verbose logs in production. Consider accepting the log level as a constructor parameter or using a less verbose default.♻️ Proposed improvement
export class FeishuClient { private readonly sdk: Lark.Client private wsClient: Lark.WSClient | null = null + private readonly loggerLevel: Lark.LoggerLevel constructor( private readonly credentials: { appId: string appSecret: string verificationToken: string encryptKey: string - } + }, + options?: { loggerLevel?: Lark.LoggerLevel } ) { + this.loggerLevel = options?.loggerLevel ?? Lark.LoggerLevel.warn // ... } // In startMessageStream: this.wsClient = new Lark.WSClient({ appId: this.credentials.appId, appSecret: this.credentials.appSecret, - loggerLevel: Lark.LoggerLevel.info + loggerLevel: this.loggerLevel })🤖 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 116 - 124, The code hardcodes Lark.LoggerLevel.info when creating wsClient, which can be too verbose for production; modify the Feishu client constructor (or the class that instantiates wsClient) to accept an optional logLevel parameter (or a config object) and use that instead of Lark.LoggerLevel.info when constructing this.wsClient in the constructor/initialize flow, defaulting to a safer level (e.g., warn or error) if none is provided; update places that instantiate the Feishu client to pass a log level when needed and ensure the new parameter is used where Lark.WSClient is constructed and in any start/init methods that reference loggerLevel.src/renderer/src/components/WindowSideBar.vue (1)
331-340: Hardcoded channel labels should use i18n keys.The tooltip builds strings with hardcoded "Telegram:" and "Feishu:" prefixes. For consistency with the i18n approach used elsewhere, consider using localized labels.
♻️ Proposed refactor
const remoteControlTooltip = computed(() => { const telegramState = remoteControlStatus.value.telegram?.enabled ? t(`chat.sidebar.remoteControlStatus.${remoteControlStatus.value.telegram.state}`) : t('chat.sidebar.remoteControlDisabled') const feishuState = remoteControlStatus.value.feishu?.enabled ? t(`chat.sidebar.remoteControlStatus.${remoteControlStatus.value.feishu.state}`) : t('chat.sidebar.remoteControlDisabled') - return [`Telegram: ${telegramState}`, `Feishu: ${feishuState}`].join('\n') + return [`${t('settings.remote.telegram.title')}: ${telegramState}`, `${t('settings.remote.feishu.title')}: ${feishuState}`].join('\n') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/WindowSideBar.vue` around lines 331 - 340, The tooltip in remoteControlTooltip currently concatenates hardcoded channel labels ("Telegram:" and "Feishu:") with localized state strings; replace those hardcoded prefixes with i18n lookups (use the existing t function) — e.g. call t('chat.sidebar.remoteControlChannel.telegram') and t('chat.sidebar.remoteControlChannel.feishu') instead of literal texts, then build the returned string the same way (joining with '\n') so remoteControlTooltip, remoteControlStatus and t are used consistently for localization.test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts (1)
93-105: Potential test flakiness with hardcoded timeout.The 25ms timeout on line 99 for the race condition test could cause flakiness in slower CI environments. Consider using a more generous timeout or a different approach to verify the async behavior.
♻️ Suggested improvement
const result = await Promise.race([ harness .onMessage({ parsed: createParsedMessage() }) .then(() => 'returned'), - new Promise((resolve) => setTimeout(() => resolve('timed-out'), 25)) + new Promise((resolve) => setTimeout(() => resolve('timed-out'), 100)) ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts` around lines 93 - 105, The test uses a fragile Promise.race with a hardcoded 25ms timeout which can flake in slow CI; replace the race-based timeout with a deterministic pattern: directly await harness.onMessage({ parsed: createParsedMessage() }) (or increase the timeout to a generous value like 200–500ms if you must keep the race), then use vi.waitFor(() => expect(harness.router.handleMessage).toHaveBeenCalledTimes(1)) to assert the async behavior; update the code around harness.onMessage, createParsedMessage, and the vi.waitFor assertion 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 `@src/main/presenter/browser/BrowserTab.ts`:
- Around line 541-557: The catch block around await
this.waitForInteractiveReady(timeoutMs) is swallowing all errors and turning
them into a generic YoBrowserNotReadyError; change it to only swallow/handle
timeout-related errors and rethrow any other error so real
navigation/page-failure exceptions are preserved. Concretely, inside the if
(this.awaitingMainFrameInteractive || this.status === BrowserPageStatus.Loading)
branch, replace the blanket catch with one that inspects the thrown error from
waitForInteractiveReady (e.g., by instanceof TimeoutError or checking an
error.code/message used by waitForInteractiveReady) and: if it is a timeout,
fall through to probeInteractiveReadiness(), otherwise rethrow the error
immediately; keep the later checks of this.interactiveReady, await
this.probeInteractiveReadiness(), and throw this.buildNotReadyError(action)
unchanged.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 265-278: The polling loop in deliverConversation currently can run
forever; add a timeout or iteration cap by recording a start timestamp (e.g.,
Date.now()) or a counter and break out if elapsed time exceeds a
MAX_CONVERSATION_POLL_MS (or iterations exceed MAX_POLL_ATTEMPTS) while using
the same TELEGRAM_STREAM_POLL_INTERVAL_MS sleep cadence; when the timeout/limit
is hit, gracefully stop by either sending a final message via
this.deps.client.sendText(target, ...) indicating the conversation timed out or
throwing/logging an error and returning—make this change inside
deliverConversation so getSnapshot/sendText and the stopRequested check remain
used but bounded.
In `@src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts`:
- Around line 90-103: The code currently uses meta?.channel or
resolveChannelFromEndpointKey(endpointKey) which can store a binding under a
channel that disagrees with what getBinding/clearBinding will later resolve;
change setBinding to always derive the channel from the endpoint key via
resolveChannelFromEndpointKey(endpointKey) (store in a local resolvedChannel),
return if resolvedChannel is falsy, and when calling updateBindings use
resolvedChannel (not meta?.channel); also ensure the stored meta for the
endpoint has its channel set/normalized to resolvedChannel (override
meta.channel if present) so getBinding and clearBinding (which use
resolveChannelFromEndpointKey) will find and remove the binding.
In `@src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts`:
- Around line 133-142: The new 'open' command route (case 'open' using
this.deps.runner.open and message produced via this.formatSessionLabel(session))
is not listed in the /help output; update the help text generator that builds
the replies for the '/help' command to include an entry for "/open" (e.g.,
"/open — open the bound session on desktop / switch to a bound session"),
ensuring the label/description matches the behavior shown in the 'open' case so
users can discover the command.
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Around line 166-240: The Telegram UI no longer exposes streamMode but persist
logic is overwriting it (causing silent flips); update the save/persist path
(e.g., persistTelegramSettings and any setter that writes telegramSettings such
as updateTelegramDefaultAgentId) to retain the existing
telegramSettings.streamMode when writing changes—read the current
telegramSettings object and copy its streamMode into the payload (or merge
changes) so streamMode is preserved until the UI explicitly exposes/modifies it
again.
- Around line 1157-1219: persistTelegramSettings and persistFeishuSettings
currently swallow errors (log/toast) so callers like
generatePairCodeAndOpenDialog, openBindingsDialog and runTelegramHookTest
continue as if the draft was saved; change these functions to surface failures
instead of swallowing them: after the catch block either rethrow the caught
error or return a rejected Promise (i.e. throw error) so callers can abort
follow-up actions, and ensure the functions still clear saving/pending in
finally; reference persistTelegramSettings, persistFeishuSettings and the save
step via saveChannelSettingsCompat when making this change.
- Around line 1505-1525: The function formatOverviewLine reads a union-typed
status (TelegramRemoteStatus | FeishuRemoteStatus) but then accesses
Telegram-only and Feishu-only properties; fix by narrowing the union before
those accesses: inside formatOverviewLine use either a guard on status.channel
(e.g. if (status.channel === 'telegram') { /* use status.allowedUserCount */ }
else { /* use status.pairedUserCount */ }) or avoid the union entirely by
reading from the component stores directly (use telegramStatus.value for the
'telegram' branch and feishuStatus.value for the Feishu branch), and keep the
existing channelStatus(channel) call to bail out when status is falsy.
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 306-330: The computed aggregatedRemoteControlState builds states
from remoteControlStatus but currently only filters on status?.enabled then
force-casts state to RemoteRuntimeState, which can allow undefined/null states
to slip through; update the filter to also validate that status.state is
non-null/defined (e.g. status?.enabled && status.state != null) or use a
type-guard filter so only entries with a defined state are mapped, and remove
the unsafe `as RemoteRuntimeState` cast when creating the states array to ensure
subsequent includes() checks operate on valid RemoteRuntimeState values.
In `@test/renderer/components/RemoteSettings.test.ts`:
- Around line 418-421: The test stubs for Tabs, TabsList, TabsTrigger, and
TabsContent are passthroughs so the suite never verifies real tab selection;
update the test to mount real tab behavior (or a minimal mock that enforces
selection) and assert visibility changes on interaction: render the actual Tabs
implementation or replace the passthroughs with a mock that tracks selected key,
simulate clicking the TabsTrigger for the Feishu tab (using the TabsTrigger
symbol) and then assert the corresponding TabsContent is visible while the other
tab panels are hidden to ensure inactive panels are gated behind selection.
---
Outside diff comments:
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Around line 1082-1123: refreshStatus and loadState use Promise.all which makes
one channel failure block the other; change these to fetch per-channel (either
via Promise.allSettled or separate try/catch awaits) so you only update
telegramStatus.value/feishuStatus.value when that channel's fetch succeeds and
preserve the previous value on failure. In refreshStatus call
getChannelStatusCompat('telegram') and getChannelStatusCompat('feishu')
independently and update telegramStatus/feishuStatus only on success; in
loadState await getChannelSettingsCompat/getChannelStatusCompat for each channel
in isolated try/catch blocks (or use Promise.allSettled and check each result)
and call syncTelegramFields/syncFeishuFields only with successful loaded
settings; also ensure loadDeepChatAgents (newAgentPresenter.getAgents) is
awaited separately so its failure doesn't block channel loads.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 1498-1509: The Persian strings mistakenly hard-code "Telegram" for
channel-aware pairing; update the keys pairDialogInstruction,
pairDialogInstructionTelegram and pairDialogInstructionFeishu in fa-IR
settings.json so the Feishu flow shows the correct channel name: either
parameterize pairDialogInstruction to accept a {channel} placeholder and
translate it (e.g., "این فرمان را برای ربات {channel} بفرستید:"), or keep
separate localized variants and ensure pairDialogInstructionFeishu uses Feishu
wording (e.g., "این فرمان را برای ربات Feishu بفرستید:"); mirror the same
approach for any binding/pairing keys that reference channels.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 1498-1509: The current translation keys pairDialogTitle and
pairDialogDescription are Telegram-specific while you added
pairDialogInstructionTelegram and pairDialogInstructionFeishu; either make the
title/description neutral or add channel-specific keys (e.g.,
pairDialogTitleTelegram / pairDialogTitleFeishu and
pairDialogDescriptionTelegram / pairDialogDescriptionFeishu) and update the
dialog rendering to use the channel-specific keys based on the pairing flow;
ensure callers that currently read pairDialogTitle/pairDialogDescription fall
back to the neutral keys if channel-specific ones are missing.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 1474-1509: The RU strings for pairing/bindings are hard-coded to
Telegram; make them channel-aware by adding separate keys for Telegram and
Feishu (or replace the generic keys with channel-specific variants) and provide
appropriate Russian copy for each. Concretely, add/replace keys such as
pairDialogTitleTelegram / pairDialogTitleFeishu, pairDialogDescriptionTelegram /
pairDialogDescriptionFeishu, bindingsDialogDescriptionTelegram /
bindingsDialogDescriptionFeishu, and bindingsEmptyTelegram / bindingsEmptyFeishu
(keeping existing pairDialogInstructionTelegram and pairDialogInstructionFeishu
patterns), then update the consumer to use the channel-specific key based on the
provider (e.g., when displaying pair dialog or bindings list, choose the
Telegram or Feishu variant).
---
Nitpick comments:
In `@docs/specs/remote-multi-channel/tasks.md`:
- Around line 8-11: Checklist misses the repo typecheck and coverage
gates—append typecheck and coverage steps to the final validation list. Update
the list item that currently reads "Run `pnpm run format`, `pnpm run i18n`,
`pnpm run lint`, and targeted Vitest suites" to also run the TypeScript
typecheck and coverage commands (e.g., add `pnpm run typecheck` and `pnpm run
test:coverage`), and note the patterns to enforce (`**/*.{js,ts,tsx}` via
OxLint/lint-staged and `test/**/*.ts` for coverage) so contributors run both
linters and the coverage/typecheck gates before completing the task.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 116-124: The code hardcodes Lark.LoggerLevel.info when creating
wsClient, which can be too verbose for production; modify the Feishu client
constructor (or the class that instantiates wsClient) to accept an optional
logLevel parameter (or a config object) and use that instead of
Lark.LoggerLevel.info when constructing this.wsClient in the
constructor/initialize flow, defaulting to a safer level (e.g., warn or error)
if none is provided; update places that instantiate the Feishu client to pass a
log level when needed and ensure the new parameter is used where Lark.WSClient
is constructed and in any start/init methods that reference loggerLevel.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 269-274: The completion branch sends snapshot.text blindly when
snapshot.completed is true; update the logic in the loop around
execution.getSnapshot() to validate snapshot.text (e.g., check for
null/undefined and trim().length > 0) before calling
this.deps.client.sendText(target, snapshot.text); if the text is empty, skip
sending (or log/notify via this.deps.logger) and then return—ensure you
reference snapshot.completed, snapshot.text, execution.getSnapshot(), and
this.deps.client.sendText(target, ...) when making the change.
In `@src/main/presenter/remoteControlPresenter/services/remoteAuthGuard.ts`:
- Around line 65-69: The compat code repeats branching logic for dispatching
bindingStoreCompat.clearPairCode; extract a small private helper (e.g.,
callClearPairCode or clearPairCodeCompat) that accepts no args and internally
checks bindingStoreCompat.clearPairCode.length to either call
bindingStoreCompat.clearPairCode() or
bindingStoreCompat.clearPairCode('telegram'), then replace both existing
branches (the block using bindingStoreCompat.clearPairCode at the two locations)
with a single call to that helper so the Telegram fallback behavior is
centralized and not duplicated.
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 331-340: The tooltip in remoteControlTooltip currently
concatenates hardcoded channel labels ("Telegram:" and "Feishu:") with localized
state strings; replace those hardcoded prefixes with i18n lookups (use the
existing t function) — e.g. call t('chat.sidebar.remoteControlChannel.telegram')
and t('chat.sidebar.remoteControlChannel.feishu') instead of literal texts, then
build the returned string the same way (joining with '\n') so
remoteControlTooltip, remoteControlStatus and t are used consistently for
localization.
In `@test/main/presenter/remoteControlPresenter/feishuCommandRouter.test.ts`:
- Around line 1-2: The test suite for FeishuCommandRouter is in the wrong
location; move this file so it mirrors the source module under the services path
(i.e., place the test alongside the implementation at
test/main/presenter/remoteControlPresenter/services/feishuCommandRouter.test.ts),
ensuring the import of FeishuCommandRouter remains valid; confirm the test
runner uses Vitest with jsdom and Vue Test Utils per project conventions and
that the filename follows the *.test.ts pattern.
In `@test/main/presenter/remoteControlPresenter/feishuParser.test.ts`:
- Around line 1-2: Test file is placed at a flattened path instead of mirroring
the source directory; move the test so its relative directory structure matches
the source (same nested folders as the module under test) and rename/locate it
to follow the test naming convention (*.test.ts or *.spec.ts). Update the test
harness to run with Vitest configured for jsdom and Vue Test Utils, and ensure
the test imports the FeishuParser symbol from its module path as in the source
so the test resolves the same module layout the source uses.
In `@test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts`:
- Around line 93-105: The test uses a fragile Promise.race with a hardcoded 25ms
timeout which can flake in slow CI; replace the race-based timeout with a
deterministic pattern: directly await harness.onMessage({ parsed:
createParsedMessage() }) (or increase the timeout to a generous value like
200–500ms if you must keep the race), then use vi.waitFor(() =>
expect(harness.router.handleMessage).toHaveBeenCalledTimes(1)) to assert the
async behavior; update the code around harness.onMessage, createParsedMessage,
and the vi.waitFor assertion accordingly.
In `@test/renderer/components/WindowSideBar.test.ts`:
- Around line 310-339: The test covers only Telegram remote-control states; add
Feishu-specific scenarios by reusing the existing setup helper (call setup with
remoteStatus plus channel flags indicating Feishu enabled, e.g., include a
channels or integrations field enabling Feishu) and add two new assertions: one
where Feishu alone is enabled (ensure
wrapper.find('[data-testid="remote-control-button"]').exists() is true and the
button classes/status text reflect Feishu) and one where both Telegram and
Feishu are enabled (ensure the button exists and the tooltip/text contains the
combined/Feishu-specific label), using the same selectors and checks as in the
existing "shows the remote control button only when remote control is enabled"
test to validate tooltip/label behavior when Feishu is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b92b9d4-b699-428e-a682-ded2cb96c022
📒 Files selected for processing (59)
docs/specs/remote-multi-channel/plan.mddocs/specs/remote-multi-channel/spec.mddocs/specs/remote-multi-channel/tasks.mdpackage.jsonsrc/main/presenter/browser/BrowserTab.tssrc/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/index.tssrc/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/feishu/feishuParser.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/interface.tssrc/main/presenter/remoteControlPresenter/services/feishuAuthGuard.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteAuthGuard.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/telegram/telegramOutbound.tssrc/main/presenter/remoteControlPresenter/types.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/settings/components/RemoteSettings.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/remote-control.presenter.d.tstest/main/presenter/browser/BrowserTab.test.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/feishuParser.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteBindingStore.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/main/presenter/remoteControlPresenter/telegramOutbound.test.tstest/main/presenter/toolPresenter/toolPresenter.test.tstest/renderer/components/RemoteSettings.test.tstest/renderer/components/WindowSideBar.test.ts
src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts
Show resolved
Hide resolved
src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts
Show resolved
Hide resolved
| <div class="border-t p-4"> | ||
| <div class="mb-3 space-y-1"> | ||
| <div class="text-sm font-medium"> | ||
| {{ t('settings.remote.sections.remoteControl') }} | ||
| </div> | ||
| <p class="text-sm text-muted-foreground"> | ||
| {{ t('settings.remote.remoteControl.description') }} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div | ||
| v-if="telegramSettings.remoteEnabled" | ||
| data-testid="remote-control-details" | ||
| class="space-y-4" | ||
| > | ||
| <div class="grid grid-cols-1 gap-4 md:grid-cols-2"> | ||
| <div class="space-y-2"> | ||
| <Label class="text-xs text-muted-foreground"> | ||
| {{ t('settings.remote.remoteControl.allowedUserIds') }} | ||
| </Label> | ||
| <Input | ||
| data-testid="remote-allowed-user-ids-input" | ||
| v-model="telegramAllowedUserIdsText" | ||
| :placeholder="t('settings.remote.remoteControl.allowedUserIdsPlaceholder')" | ||
| @blur="persistTelegramSettings" | ||
| /> | ||
| </div> | ||
|
|
||
| <div class="space-y-2"> | ||
| <Label class="text-xs text-muted-foreground"> | ||
| {{ t('settings.remote.remoteControl.defaultAgent') }} | ||
| </Label> | ||
| <Select | ||
| :model-value="telegramSettings.defaultAgentId" | ||
| @update:model-value="(value) => updateTelegramDefaultAgentId(String(value))" | ||
| > | ||
| <SelectTrigger data-testid="remote-default-agent-select" class="h-8!"> | ||
| <SelectValue | ||
| :placeholder=" | ||
| t('settings.remote.remoteControl.defaultAgentPlaceholder') | ||
| " | ||
| /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem | ||
| v-for="agent in defaultAgentOptions(telegramSettings.defaultAgentId)" | ||
| :key="agent.id" | ||
| :value="agent.id" | ||
| > | ||
| {{ agent.name }} | ||
| </SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="flex flex-wrap items-center gap-2"> | ||
| <Button | ||
| data-testid="remote-pair-button" | ||
| variant="outline" | ||
| size="sm" | ||
| @click="generatePairCodeAndOpenDialog('telegram')" | ||
| > | ||
| {{ t('settings.remote.remoteControl.openPairDialog') }} | ||
| </Button> | ||
| <Button | ||
| data-testid="remote-bindings-button" | ||
| variant="outline" | ||
| size="sm" | ||
| @click="openBindingsDialog('telegram')" | ||
| > | ||
| {{ t('settings.remote.remoteControl.manageBindings') }} | ||
| </Button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Preserve Telegram streamMode until this section exposes it again.
The Telegram panel no longer renders a stream-mode control, and the main presenter now persists Telegram settings back as draft on save. A user who previously chose final will be silently flipped the next time they change bot token, allowlist, or default agent here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/settings/components/RemoteSettings.vue` around lines 166 - 240,
The Telegram UI no longer exposes streamMode but persist logic is overwriting it
(causing silent flips); update the save/persist path (e.g.,
persistTelegramSettings and any setter that writes telegramSettings such as
updateTelegramDefaultAgentId) to retain the existing telegramSettings.streamMode
when writing changes—read the current telegramSettings object and copy its
streamMode into the payload (or merge changes) so streamMode is preserved until
the UI explicitly exposes/modifies it again.
| const formatOverviewLine = (channel: RemoteChannel) => { | ||
| const status = channelStatus(channel) | ||
| if (!status) { | ||
| return '' | ||
| } | ||
|
|
||
| if (channel === 'telegram') { | ||
| return t('settings.remote.overview.telegram', { | ||
| bindingCount: status.bindingCount, | ||
| pairedCount: status.allowedUserCount, | ||
| hooks: telegramSettings.value?.hookNotifications.enabled | ||
| ? t('settings.remote.overview.hooksOn') | ||
| : t('settings.remote.overview.hooksOff') | ||
| }) | ||
| } | ||
|
|
||
| return t('settings.remote.overview.feishu', { | ||
| bindingCount: status.bindingCount, | ||
| pairedCount: status.pairedUserCount | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the RemoteSettings.vue file
cd src/renderer/settings/components
ls -la RemoteSettings.vue
wc -l RemoteSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 157
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1505,1525p' src/renderer/settings/components/RemoteSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 672
🏁 Script executed:
# Look for type definitions and channelStatus function
rg -n "type RemoteChannel|interface RemoteChannel" --type vue --type tsRepository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
# Find channelStatus function definition
rg -n "const channelStatus|function channelStatus" --type vue --type tsRepository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
# Look for TelegramRemoteStatus and FeishuRemoteStatus types
rg -n "TelegramRemoteStatus|FeishuRemoteStatus" --type vue --type tsRepository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
# Search for type definitions more broadly
rg "type RemoteChannel|interface RemoteChannel"Repository: ThinkInAIXYZ/deepchat
Length of output: 446
🏁 Script executed:
# Search for channelStatus function definition
rg "channelStatus" --max-count 10Repository: ThinkInAIXYZ/deepchat
Length of output: 908
🏁 Script executed:
# Look for TelegramRemoteStatus and FeishuRemoteStatus
rg "TelegramRemoteStatus|FeishuRemoteStatus" --max-count 15Repository: ThinkInAIXYZ/deepchat
Length of output: 3471
🏁 Script executed:
# Search within RemoteSettings.vue for imports and type definitions
head -100 src/renderer/settings/components/RemoteSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 4020
🏁 Script executed:
# Find the channelStatus function definition
rg "const channelStatus = " -A 5 src/renderer/settings/components/RemoteSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 304
🏁 Script executed:
# Get full type definitions
cat src/shared/types/presenters/remote-control.presenter.d.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 4472
🏁 Script executed:
# Check for the full function around line 1505-1525 with more context
sed -n '1490,1540p' src/renderer/settings/components/RemoteSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 1353
formatOverviewLine() accesses channel-specific fields without type narrowing.
The status variable has type TelegramRemoteStatus | FeishuRemoteStatus, but narrowing the channel parameter doesn't narrow status. Line 1514 accesses status.allowedUserCount (Telegram-only) and line 1523 accesses status.pairedUserCount (Feishu-only), causing TypeScript errors. Narrow the union by checking status.channel or access telegramStatus.value and feishuStatus.value directly within each branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/settings/components/RemoteSettings.vue` around lines 1505 -
1525, The function formatOverviewLine reads a union-typed status
(TelegramRemoteStatus | FeishuRemoteStatus) but then accesses Telegram-only and
Feishu-only properties; fix by narrowing the union before those accesses: inside
formatOverviewLine use either a guard on status.channel (e.g. if (status.channel
=== 'telegram') { /* use status.allowedUserCount */ } else { /* use
status.pairedUserCount */ }) or avoid the union entirely by reading from the
component stores directly (use telegramStatus.value for the 'telegram' branch
and feishuStatus.value for the Feishu branch), and keep the existing
channelStatus(channel) call to bail out when status is falsy.
| const aggregatedRemoteControlState = computed<RemoteRuntimeState>(() => { | ||
| const states = [remoteControlStatus.value.telegram, remoteControlStatus.value.feishu] | ||
| .filter((status) => status?.enabled) | ||
| .map((status) => status?.state as RemoteRuntimeState) | ||
|
|
||
| if (states.length === 0) { | ||
| return 'disabled' | ||
| } | ||
| if (states.includes('error')) { | ||
| return 'error' | ||
| } | ||
| if (states.includes('backoff')) { | ||
| return 'backoff' | ||
| } | ||
| if (states.includes('starting')) { | ||
| return 'starting' | ||
| } | ||
| if (states.includes('running')) { | ||
| return 'running' | ||
| } | ||
| if (states.includes('stopped')) { | ||
| return 'stopped' | ||
| } | ||
| return 'disabled' | ||
| }) |
There was a problem hiding this comment.
Potential type safety issue with undefined state.
On line 309, the type assertion as RemoteRuntimeState is applied after filtering only on status?.enabled. If a channel is enabled but its state property is undefined/null, this could lead to unexpected behavior in the subsequent includes() checks.
🛡️ Proposed fix to add state validation
const aggregatedRemoteControlState = computed<RemoteRuntimeState>(() => {
const states = [remoteControlStatus.value.telegram, remoteControlStatus.value.feishu]
- .filter((status) => status?.enabled)
- .map((status) => status?.state as RemoteRuntimeState)
+ .filter((status): status is NonNullable<typeof status> => Boolean(status?.enabled && status?.state))
+ .map((status) => status.state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/WindowSideBar.vue` around lines 306 - 330, The
computed aggregatedRemoteControlState builds states from remoteControlStatus but
currently only filters on status?.enabled then force-casts state to
RemoteRuntimeState, which can allow undefined/null states to slip through;
update the filter to also validate that status.state is non-null/defined (e.g.
status?.enabled && status.state != null) or use a type-guard filter so only
entries with a defined state are mapped, and remove the unsafe `as
RemoteRuntimeState` cast when creating the states array to ensure subsequent
includes() checks operate on valid RemoteRuntimeState values.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/renderer/settings/components/RemoteSettings.vue (1)
1535-1555:⚠️ Potential issue | 🟡 Minor
formatOverviewLine()accesses channel-specific fields without proper type narrowing.The
statusvariable fromchannelStatus(channel)has typeTelegramRemoteStatus | FeishuRemoteStatus | null. Narrowing via thechannelparameter doesn't narrowstatus. Line 1544 accessesstatus.allowedUserCount(Telegram-only) and line 1553 accessesstatus.pairedUserCount(Feishu-only). While this works at runtime, TypeScript cannot verify type safety.🔧 Suggested fix using direct refs
const formatOverviewLine = (channel: RemoteChannel) => { - const status = channelStatus(channel) - if (!status) { - return '' - } - if (channel === 'telegram') { + const status = telegramStatus.value + if (!status) { + return '' + } return t('settings.remote.overview.telegram', { bindingCount: status.bindingCount, pairedCount: status.allowedUserCount, hooks: telegramSettings.value?.hookNotifications.enabled ? t('settings.remote.overview.hooksOn') : t('settings.remote.overview.hooksOff') }) } + const status = feishuStatus.value + if (!status) { + return '' + } return t('settings.remote.overview.feishu', { bindingCount: status.bindingCount, pairedCount: status.pairedUserCount }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/RemoteSettings.vue` around lines 1535 - 1555, formatOverviewLine uses a union-typed status (TelegramRemoteStatus | FeishuRemoteStatus | null) but narrows only by channel, so TypeScript can't verify property accesses like status.allowedUserCount and status.pairedUserCount; fix by performing an explicit type-narrowing guard after calling channelStatus(channel) (e.g., add an isTelegramStatus(type guard) or check for a discriminating property like 'allowedUserCount' on status) and then access Telegram-only fields inside the telegram branch and Feishu-only fields inside the feishu branch, referencing formatOverviewLine, channelStatus, RemoteChannel, TelegramRemoteStatus, FeishuRemoteStatus and telegramSettings to locate the code.
🧹 Nitpick comments (2)
test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts (1)
202-225: Add a route test for/open, not just help text.This update only proves
/openis listed. The new command path itself still is not exercised, so regressions inrunner.open()wiring or its reply text would slip through. A couple of cases here for “opened session” and thenullresult path would make the feature much safer to change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts` around lines 202 - 225, Add explicit tests that exercise the /open command route (not just that it appears in /help): create a RemoteCommandRouter in the test with a mocked runner that implements open (e.g., runner.open = vi.fn()) and call router.handleMessage with createMessage({ text: '/open <id>', command: { name: 'open', args: '<id>' } }). Add two cases: one where runner.open resolves to a non-null session object and assert result.replies include the "opened session" success text and any session identifier, and one where runner.open resolves to null and assert the null/failure reply is returned. Use the same helpers (createMessage, RemoteCommandRouter) and assert on result.replies[0]; ensure you reset/verify the runner.open mock per test.src/main/presenter/remoteControlPresenter/index.ts (1)
715-746: Use the channel-specific constant when handling the Feishu channel.Lines 727–729 use
TELEGRAM_REMOTE_DEFAULT_AGENT_IDfor both Telegram and Feishu fallbacks. While both constants hold the same value ('deepchat'), the codebase establishes a pattern of using channel-specific constants elsewhere (e.g.,FEISHU_REMOTE_DEFAULT_AGENT_IDintypes.tslines 414, 809). Import and useFEISHU_REMOTE_DEFAULT_AGENT_IDwhen the channel is'feishu'for consistency.🤖 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 715 - 746, In sanitizeDefaultAgentId, the Feishu branch currently uses TELEGRAM_REMOTE_DEFAULT_AGENT_ID for fallback; change it to use the channel-specific constant FEISHU_REMOTE_DEFAULT_AGENT_ID: update the normalization and nextDefaultAgentId logic so when channel === 'feishu' you prefer FEISHU_REMOTE_DEFAULT_AGENT_ID (and import FEISHU_REMOTE_DEFAULT_AGENT_ID from types.ts), and ensure the Feishu config update (bindingStore.updateFeishuConfig / getFeishuDefaultAgentId) uses the computed nextDefaultAgentId as before.
🤖 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/browser/BrowserTab.ts`:
- Around line 117-125: The code calls beginMainFrameNavigation() unconditionally
after session.sendCommand for 'Page.navigate', causing same-document or failed
navigations to reset interactiveReady/fullReady; update the handler to inspect
the navigation response (the response variable returned from
session.sendCommand) and only call beginMainFrameNavigation(...) when
response.loaderId is present/non-empty and response.errorText is absent (i.e.,
successful cross-document navigation); keep the existing URL selection logic
(typeof params?.url ... ? params.url : this.url) but guard that call behind the
loaderId && !errorText check so fragment/history updates and failed navigations
do not reset loading state.
- Around line 834-847: The probe currently lets document.body alone flip the tab
to interactive; change the logic in BrowserTab (the block using probe.readyState
and probe.hasBody) so that readyState is enforced first: if readyState is not
'interactive' or 'complete' return false immediately, and only after that
proceed to check hasBody (and return false if hasBody !== true), then set
awaitingMainFrameInteractive/interactiveReady/lastInteractiveAt/updatedAt and
update this.url; in short, remove the current combined condition that allows
hasBody to bypass readyState and replace it with a strict readyState check
followed by the body check.
- Around line 888-894: BrowserTab currently only updates this.url on main-frame
navigations in the did-start-navigation listener; add a handler for the Electron
'did-navigate-in-page' event on this.webContents to update in-page
(hash/history) navigations so this.url stays synchronized. In that handler call
the same update logic used for main-frame navigations (e.g., invoke
beginMainFrameNavigation(details.url || this.url) or otherwise set this.url) so
toPageInfo and diagnostics reflect the current URL; reference BrowserTab,
this.webContents, beginMainFrameNavigation, this.url, and toPageInfo when making
the change.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 47-57: The runtime can restart while promises in
endpointOperations are still resolving, allowing stale work to act under a new
start; introduce a generation token (e.g., private runId = 0 and an
isCurrentRun(runId: number) helper) and capture const runId = ++this.runId in
start(), increment runId again in stop(), and thread that token into
acceptRawMessage, enqueueEndpointOperation, processInboundMessage, and
deliverConversation so every async path checks isCurrentRun(runId) before
proceeding or sending replies; also ensure any queued/delivered operations are
gated by this token and that stop() either drains outstanding work or bumps
runId to invalidate in-flight promises.
In `@src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts`:
- Around line 134-140: The reply for the 'open' command conflates two different
failure modes because RemoteConversationRunner.open currently returns null for
both "no bound session" and "desktop window cannot be resolved"; change
RemoteConversationRunner.open to return a small discriminated result (e.g. {
status: 'noSession' } | { status: 'windowNotFound' } | { status: 'ok', session
}) and then update remoteCommandRouter's case 'open' to inspect that result
(instead of truthiness) and return the appropriate guidance messages (use /new
or /use for 'noSession', a window-not-found hint for 'windowNotFound', and the
formatted success message using formatSessionLabel when status is 'ok').
---
Duplicate comments:
In `@src/renderer/settings/components/RemoteSettings.vue`:
- Around line 1535-1555: formatOverviewLine uses a union-typed status
(TelegramRemoteStatus | FeishuRemoteStatus | null) but narrows only by channel,
so TypeScript can't verify property accesses like status.allowedUserCount and
status.pairedUserCount; fix by performing an explicit type-narrowing guard after
calling channelStatus(channel) (e.g., add an isTelegramStatus(type guard) or
check for a discriminating property like 'allowedUserCount' on status) and then
access Telegram-only fields inside the telegram branch and Feishu-only fields
inside the feishu branch, referencing formatOverviewLine, channelStatus,
RemoteChannel, TelegramRemoteStatus, FeishuRemoteStatus and telegramSettings to
locate the code.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/index.ts`:
- Around line 715-746: In sanitizeDefaultAgentId, the Feishu branch currently
uses TELEGRAM_REMOTE_DEFAULT_AGENT_ID for fallback; change it to use the
channel-specific constant FEISHU_REMOTE_DEFAULT_AGENT_ID: update the
normalization and nextDefaultAgentId logic so when channel === 'feishu' you
prefer FEISHU_REMOTE_DEFAULT_AGENT_ID (and import FEISHU_REMOTE_DEFAULT_AGENT_ID
from types.ts), and ensure the Feishu config update
(bindingStore.updateFeishuConfig / getFeishuDefaultAgentId) uses the computed
nextDefaultAgentId as before.
In `@test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts`:
- Around line 202-225: Add explicit tests that exercise the /open command route
(not just that it appears in /help): create a RemoteCommandRouter in the test
with a mocked runner that implements open (e.g., runner.open = vi.fn()) and call
router.handleMessage with createMessage({ text: '/open <id>', command: { name:
'open', args: '<id>' } }). Add two cases: one where runner.open resolves to a
non-null session object and assert result.replies include the "opened session"
success text and any session identifier, and one where runner.open resolves to
null and assert the null/failure reply is returned. Use the same helpers
(createMessage, RemoteCommandRouter) and assert on result.replies[0]; ensure you
reset/verify the runner.open mock per test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc088706-ecfc-49e6-ac5e-332274c955a0
📒 Files selected for processing (16)
src/main/presenter/browser/BrowserTab.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/services/feishuAuthGuard.tssrc/main/presenter/remoteControlPresenter/services/remoteAuthGuard.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/types.tssrc/renderer/settings/components/RemoteSettings.vuetest/main/presenter/remoteControlPresenter/feishuAuthGuard.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteAuthGuard.test.tstest/main/presenter/remoteControlPresenter/remoteBindingStore.test.tstest/main/presenter/remoteControlPresenter/remoteCommandRouter.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/renderer/components/RemoteSettings.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/main/presenter/remoteControlPresenter/remoteControlPresenter.test.ts
- src/main/presenter/remoteControlPresenter/services/remoteAuthGuard.ts
- test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts
- src/main/presenter/remoteControlPresenter/types.ts
| case 'open': { | ||
| const session = await this.deps.runner.open(endpointKey) | ||
| return { | ||
| replies: [ | ||
| session | ||
| ? `Opened on desktop: ${this.formatSessionLabel(session)}` | ||
| : 'No bound session. Send a message, /new, or /use first.' |
There was a problem hiding this comment.
Differentiate /open failure reasons.
RemoteConversationRunner.open() currently returns null both when there is no bound session and when the desktop window cannot be resolved (src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts:108-124). This reply will tell users to /new or /use even when a session is already bound, which is misleading. Returning a small status/result from open() would let this route send the right guidance for each case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts`
around lines 134 - 140, The reply for the 'open' command conflates two different
failure modes because RemoteConversationRunner.open currently returns null for
both "no bound session" and "desktop window cannot be resolved"; change
RemoteConversationRunner.open to return a small discriminated result (e.g. {
status: 'noSession' } | { status: 'windowNotFound' } | { status: 'ok', session
}) and then update remoteCommandRouter's case 'open' to inspect that result
(instead of truthiness) and return the appropriate guidance messages (use /new
or /use for 'noSession', a window-not-found hint for 'windowNotFound', and the
formatted success message using formatSessionLabel when status is 'ok').
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
107-109: Consider clearing stale status fields when transitioning tostopped.
stop()currently updates onlystate, so oldbotUser/lastErrorcan remain visible in snapshots after shutdown.♻️ Suggested adjustment
this.setStatus({ - state: 'stopped' + state: 'stopped', + lastError: null, + botUser: null })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts` around lines 107 - 109, The stop() transition only sets state to 'stopped' leaving stale fields like botUser and lastError populated; update the call to setStatus in stop() (in feishuRuntime.ts) to clear or reset those fields as well (e.g., include botUser: null and lastError: undefined or empty) so snapshots reflect a fully stopped state while preserving the use of setStatus for status updates.test/main/presenter/browser/BrowserTab.test.ts (1)
253-261: Extract duplicatedPage.navigatemock setup into a helper.The same
sendCommandmock implementation is repeated three times. A local helper will keep these tests easier to evolve and less error-prone.♻️ Proposed refactor
+ const mockPageNavigate = (webContents: MockWebContents) => { + webContents.debugger.sendCommand.mockImplementation(async (method: string, params?: any) => { + if (method === 'Page.navigate') { + if (typeof params?.url === 'string') { + webContents.url = params.url + } + return { frameId: 'frame-1', loaderId: 'loader-1' } + } + return {} + }) + } - webContents.debugger.sendCommand.mockImplementation(async (method: string, params?: any) => { - if (method === 'Page.navigate') { - if (typeof params?.url === 'string') { - webContents.url = params.url - } - return { frameId: 'frame-1', loaderId: 'loader-1' } - } - return {} - }) + mockPageNavigate(webContents)Also applies to: 288-296, 321-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/browser/BrowserTab.test.ts` around lines 253 - 261, Extract the duplicated sendCommand mock into a single helper (e.g., setupPageNavigateMock) that calls webContents.debugger.sendCommand.mockImplementation with the Page.navigate behavior shown (set webContents.url when params.url is string and return the { frameId, loaderId } object, otherwise return {}), then replace the three inline mockImplementation blocks in BrowserTab.test.ts with calls to that helper; ensure the helper is imported/declared in the test scope so type signatures and the original behavior (handling method === 'Page.navigate') are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 275-282: The code sends raw error.message to Feishu via
this.deps.client.sendText (inside the isCurrentRun(runId) guard), which may leak
sensitive internals; replace the forwarded text with a generic user-safe message
like "An internal error occurred while processing your request." and ensure the
detailed error is recorded to diagnostics (use this.deps.logger.error(error, {
runId, target }) or fallback to console.error) so developers can debug without
exposing internals to Feishu users.
In `@test/main/presenter/browser/BrowserTab.test.ts`:
- Around line 57-70: The mock's emitStartNavigation currently emits navigation
metadata as event arguments for 'did-start-navigation', but Electron only passes
the event object; update emitStartNavigation (and any listeners in the tests) to
emit only the event object (no url/isSameDocument/isMainFrame arguments) and
rely on the mock instance properties (e.g., this.url) to store navigation
metadata for test assertions; ensure listeners read navigation details from the
mock (this.url, this.isSameDocument, this.isMainFrame if added) instead of
expecting them as event parameters.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 107-109: The stop() transition only sets state to 'stopped'
leaving stale fields like botUser and lastError populated; update the call to
setStatus in stop() (in feishuRuntime.ts) to clear or reset those fields as well
(e.g., include botUser: null and lastError: undefined or empty) so snapshots
reflect a fully stopped state while preserving the use of setStatus for status
updates.
In `@test/main/presenter/browser/BrowserTab.test.ts`:
- Around line 253-261: Extract the duplicated sendCommand mock into a single
helper (e.g., setupPageNavigateMock) that calls
webContents.debugger.sendCommand.mockImplementation with the Page.navigate
behavior shown (set webContents.url when params.url is string and return the {
frameId, loaderId } object, otherwise return {}), then replace the three inline
mockImplementation blocks in BrowserTab.test.ts with calls to that helper;
ensure the helper is imported/declared in the test scope so type signatures and
the original behavior (handling method === 'Page.navigate') are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84357a6e-964e-42e9-9de1-84ed17afef87
📒 Files selected for processing (10)
src/main/presenter/browser/BrowserTab.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tstest/main/presenter/browser/BrowserTab.test.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteCommandRouter.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/main/presenter/remoteControlPresenter/remoteCommandRouter.test.ts
- src/main/presenter/remoteControlPresenter/services/remoteCommandRouter.ts
- src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts
- test/main/presenter/remoteControlPresenter/feishuCommandRouter.test.ts
- test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts
- src/main/presenter/browser/BrowserTab.ts
src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts
Outdated
Show resolved
Hide resolved
| emitStartNavigation( | ||
| url: string, | ||
| options?: { | ||
| isSameDocument?: boolean | ||
| isMainFrame?: boolean | ||
| } | ||
| ) { | ||
| this.url = url | ||
| this.emit('did-start-navigation', { | ||
| url, | ||
| isSameDocument: options?.isSameDocument ?? false, | ||
| isMainFrame: options?.isMainFrame ?? true | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
browser_tab_file=$(fd '^BrowserTab\.ts$')
browser_tab_test_file=$(fd '^BrowserTab\.test\.ts$')
echo "Inspecting navigation listener signatures:"
echo " source: $browser_tab_file"
echo " test: $browser_tab_test_file"
rg -n -C3 "did-start-navigation|did-navigate-in-page" "$browser_tab_file" "$browser_tab_test_file"Repository: ThinkInAIXYZ/deepchat
Length of output: 1796
🌐 Web query:
Electron webContents did-start-navigation event signature arguments
💡 Result:
The 'did-start-navigation' event on Electron's webContents has no arguments other than the standard Event object. Usage example: win.webContents.on('did-start-navigation', (event) => { console.log('Navigation started'); }); This event fires when a navigation starts, for both document navigations and in-page navigations. It is not cancellable. Official Electron documentation lists it under Navigation Events but does not specify additional parameters, unlike events like 'will-navigate' (event, url) or 'did-navigate' (event, url). Older documentation versions (e.g., v1.8) and GitHub issues confirm it takes only the event parameter, consistent with Chromium's FrameNavigated event timing before details are available.
Citations:
- 1: https://electronjs.org/docs/latest/api/web-contents
- 2: https://www.electronjs.org/docs/api/web-contents
- 3: https://www.electronjs.org/docs/latest/api/web-contents
- 4: https://github.com/electron/electron/blob/v1.8.2-beta.3/docs/api/web-contents.md
Mocked did-start-navigation payload does not match Electron's event signature.
Electron's did-start-navigation event receives only the event object—no additional parameters. The test mock incorrectly emits navigation details as event arguments, which will not occur in production and weakens test validity.
🔧 Proposed fix
emitStartNavigation(
url: string,
options?: {
isSameDocument?: boolean
isMainFrame?: boolean
}
) {
this.url = url
- this.emit('did-start-navigation', {
- url,
- isSameDocument: options?.isSameDocument ?? false,
- isMainFrame: options?.isMainFrame ?? true
- })
+ this.emit('did-start-navigation')
}If navigation metadata (url, isSameDocument, isMainFrame) is needed by listeners, it should be stored on the mock object itself (as already done with this.url) and accessed directly rather than passed as event arguments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| emitStartNavigation( | |
| url: string, | |
| options?: { | |
| isSameDocument?: boolean | |
| isMainFrame?: boolean | |
| } | |
| ) { | |
| this.url = url | |
| this.emit('did-start-navigation', { | |
| url, | |
| isSameDocument: options?.isSameDocument ?? false, | |
| isMainFrame: options?.isMainFrame ?? true | |
| }) | |
| } | |
| emitStartNavigation( | |
| url: string, | |
| options?: { | |
| isSameDocument?: boolean | |
| isMainFrame?: boolean | |
| } | |
| ) { | |
| this.url = url | |
| this.emit('did-start-navigation', { | |
| url, | |
| isSameDocument: options?.isSameDocument ?? false, | |
| isMainFrame: options?.isMainFrame ?? true | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/presenter/browser/BrowserTab.test.ts` around lines 57 - 70, The
mock's emitStartNavigation currently emits navigation metadata as event
arguments for 'did-start-navigation', but Electron only passes the event object;
update emitStartNavigation (and any listeners in the tests) to emit only the
event object (no url/isSameDocument/isMainFrame arguments) and rely on the mock
instance properties (e.g., this.url) to store navigation metadata for test
assertions; ensure listeners read navigation details from the mock (this.url,
this.isSameDocument, this.isMainFrame if added) instead of expecting them as
event parameters.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts (1)
112-114: Clear stale error state when moving tostopped.Explicit stop currently preserves previous
lastError, which can make status consumers show a stale failure while already stopped.Suggested diff
this.setStatus({ - state: 'stopped' + state: 'stopped', + lastError: null })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts` around lines 112 - 114, When setting the runtime to stopped in feishuRuntime.ts, clear any stale error by passing lastError: undefined (or null depending on setStatus usage) to setStatus so the previous failure isn't shown while stopped; update the call site that currently does this.setStatus({ state: 'stopped' }) to include lastError cleared and ensure other callers of setStatus expect/handle lastError being undefined (reference setStatus and the 'stopped' state in FeishuRuntime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 112-114: When setting the runtime to stopped in feishuRuntime.ts,
clear any stale error by passing lastError: undefined (or null depending on
setStatus usage) to setStatus so the previous failure isn't shown while stopped;
update the call site that currently does this.setStatus({ state: 'stopped' }) to
include lastError cleared and ensure other callers of setStatus expect/handle
lastError being undefined (reference setStatus and the 'stopped' state in
FeishuRuntime).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7faa01b1-6c15-4909-8590-3e5a42690dda
📒 Files selected for processing (3)
src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/index.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/presenter/remoteControlPresenter/index.ts
- test/main/presenter/remoteControlPresenter/feishuRuntime.test.ts
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Localization