Conversation
Contributor
📝 WalkthroughWalkthroughAdds RTK (Runtime Toolkit) support: new runtime service with discovery/health/retry, injects RTK into install scripts, integrates RTK command rewriting into execution paths (bash, skills), threads RTK metadata through presenters/tools, adds dashboard UI/i18n and floating-widget features, and includes extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AgentBash as AgentBashHandler
participant RTK as rtkRuntimeService
participant Shell as ShellProcess
participant Fallback as FallbackHandler
User->>AgentBash: executeCommand(command)
AgentBash->>RTK: prepareShellCommand(command)
alt RTK available & healthy
RTK-->>AgentBash: {command: rewritten, env, rewritten: true}
AgentBash->>Shell: execute(rewritten command, env)
alt rewrite succeeds
Shell-->>AgentBash: output
AgentBash-->>User: {output, rtkApplied: true, rtkMode: "rewrite"}
else rewrite fails
Shell-->>AgentBash: error
AgentBash->>Fallback: determineFallbackReason(error)
Fallback-->>AgentBash: fallbackReason
AgentBash->>Shell: execute(original command, original env)
Shell-->>AgentBash: output
AgentBash-->>User: {output, rtkApplied: false, rtkMode: "bypass", rtkFallbackReason}
end
else RTK unavailable/unhealthy
RTK-->>AgentBash: {command: original, env, rewritten: false}
AgentBash->>Shell: execute(original command, env)
Shell-->>AgentBash: output
AgentBash-->>User: {output, rtkApplied: false, rtkMode: "bypass"}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 704e492da5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dbPath?: string | ||
| ): Promise<Record<string, string>> { | ||
| const shellEnv = await this.getShellEnvironmentImpl() | ||
| const env = this.runtimeHelper.prependBundledRuntimeToEnv({ |
There was a problem hiding this comment.
When the bundled RTK exists but is unhealthy, the health check is supposed to fall back to the system candidate (command: 'rtk' in resolveRuntimeCandidates). However createRuntimeEnv always prepends bundled runtime paths, so verifyRuntimeCandidate resolves rtk from the bundled directory again instead of the system install. In that scenario users with a working system RTK will still be marked unhealthy and RTK remains unusable.
Useful? React with 👍 / 👎.
![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: 10
🧹 Nitpick comments (8)
src/renderer/src/i18n/zh-TW/settings.json (1)
279-285: Use Traditional Chinese (Taiwan) terminology for consistency.A few terms in this block use Simplified Chinese conventions instead of Traditional Chinese (Taiwan) conventions:
- Line 279:
"bundled": "內置"→ should be"內建"(consistent with line 951"builtIn": "內建"elsewhere in this file)- Line 285:
"commands": "已跟蹤命令"→ should be"已追蹤命令"(Taiwan uses "追蹤" instead of "跟蹤")🌐 Suggested fix for zh-TW terminology
"source": { - "bundled": "內置", + "bundled": "內建", "system": "系統", "none": "不可用" }, "summary": { "savedTokens": "節省 Token", - "commands": "已跟蹤命令", + "commands": "已追蹤命令", "avgSavingsPct": "平均節省率", "outputTokens": "過濾後輸出" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-TW/settings.json` around lines 279 - 285, Update the Traditional Chinese translations in the settings JSON: change the value for the "bundled" key from "內置" to "內建" to match the "builtIn" terminology used elsewhere, and change the value for the "commands" key from "已跟蹤命令" to "已追蹤命令" to use the Taiwan-preferred term; make these edits in the same object that contains "bundled" and "summary" so the file remains consistent with the "builtIn": "內建" usage.package.json (1)
47-53: Keep runtime cleanup in sync with new RTK injection.Line 47-Line 53 now provision
runtime/rtk, butcleanRuntimedoes not remove it (and also missesruntime/ripgrep). Please keep install/clean scripts aligned to avoid stale runtime artifacts.Suggested patch
- "cleanRuntime": "rm -rf runtime/uv runtime/node", + "cleanRuntime": "rm -rf runtime/uv runtime/node runtime/ripgrep runtime/rtk",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 47 - 53, The package.json added provisioning for runtime/rtk and runtime/ripgrep in scripts like "installRuntime" and the platform variants ("installRuntime:win:x64", "installRuntime:mac:arm64", etc.), but the cleanup scripts (e.g., "cleanRuntime" and any platform-specific clean scripts) were not updated; modify the clean scripts to remove ./runtime/rtk and ./runtime/ripgrep (add rm -rf ./runtime/rtk and rm -rf ./runtime/ripgrep alongside the existing removals) so all install and clean script sets are symmetrical.src/renderer/src/components/message/MessageBlockToolCall.vue (1)
17-23: Consider using i18n for the RTK badge text.The badge text "RTK" is hardcoded. Per coding guidelines, user-facing strings should use vue-i18n keys. While "RTK" is a technical acronym that may not need translation, using i18n would maintain consistency.
♻️ Suggested refactor
<span v-if="showRtkBadge" data-testid="tool-call-rtk-badge" class="shrink-0 rounded border border-emerald-500/20 bg-emerald-500/10 px-1.5 py-0.5 text-[10px] font-semibold uppercase tracking-[0.08em] text-emerald-700 dark:text-emerald-300" > - RTK + {{ t('toolCall.rtkBadge') }} </span>Add to i18n files:
{ "toolCall": { "rtkBadge": "RTK" } }As per coding guidelines:
src/renderer/src/**/*.{vue,ts,tsx}: Use vue-i18n keys fromsrc/renderer/src/i18nfor all user-facing strings; never hardcode UI text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/message/MessageBlockToolCall.vue` around lines 17 - 23, The RTK badge text in MessageBlockToolCall.vue is hardcoded in the span (v-if="showRtkBadge" with data-testid="tool-call-rtk-badge"); replace the literal "RTK" with a vue-i18n lookup (e.g. use $t('toolCall.rtkBadge') or the composition API equivalent) and add the key "toolCall.rtkBadge": "RTK" to the project's i18n JSON files so the UI text comes from localization resources; keep the existing data-testid and class attributes unchanged.test/main/lib/agentRuntime/rtkRuntimeService.test.ts (1)
26-36: Consider exposing test helpers instead of directly mutating private state.The
as nevercasts to access privatehealthStateandresolvedRuntimeproperties create brittle tests that may break if internal implementation changes. Consider adding a package-private test factory or mock constructor that accepts initial state.♻️ Alternative: Expose test-only factory method
In
rtkRuntimeService.ts:// For testing only - consider moving to a separate test utility static _createWithState( deps: RtkRuntimeServiceDeps, healthState: HealthState, resolvedRuntime: ResolvedRuntime ): RtkRuntimeService { const service = new RtkRuntimeService(deps) service.healthState = healthState service.resolvedRuntime = resolvedRuntime return service }This makes the test intent explicit and avoids the
as neverescape hatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/lib/agentRuntime/rtkRuntimeService.test.ts` around lines 26 - 36, Tests are mutating private fields via "as never" (healthState, resolvedRuntime) on RtkRuntimeService; add a package-private test factory on RtkRuntimeService (e.g., static _createWithState(deps: RtkRuntimeServiceDeps, healthState: HealthState, resolvedRuntime: ResolvedRuntime): RtkRuntimeService) that constructs an instance and sets those internal fields, update tests to use RtkRuntimeService._createWithState(...) instead of casting to never, and keep the factory marked/test-only so production API is unchanged.src/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.ts (1)
15-20: Consider using proper interface typing instead of inline type assertion.The
as unknown aspattern bypasses type safety. Consider extending the presenter interface to includestartRtkHealthCheckas an optional method, or create a type guard for cleaner runtime checking.♻️ Optional: Type guard approach
+function hasRtkHealthCheck( + presenter: unknown +): presenter is { startRtkHealthCheck: () => Promise<void> } { + return ( + typeof presenter === 'object' && + presenter !== null && + 'startRtkHealthCheck' in presenter && + typeof (presenter as Record<string, unknown>).startRtkHealthCheck === 'function' + ) +} + export const rtkHealthCheckHook: LifecycleHook = { // ... execute: async (_context: LifecycleContext) => { if (!presenter) { throw new Error('rtkHealthCheckHook: Presenter not initialized') } - const newAgentPresenter = presenter.newAgentPresenter as unknown as { - startRtkHealthCheck?: () => Promise<void> - } - if (!newAgentPresenter.startRtkHealthCheck) { + if (!hasRtkHealthCheck(presenter.newAgentPresenter)) { return } - void newAgentPresenter.startRtkHealthCheck().catch((error) => { + void presenter.newAgentPresenter.startRtkHealthCheck().catch((error) => { console.error('rtkHealthCheckHook: failed to start RTK health check:', error) }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.ts` around lines 15 - 20, The code uses an unsafe "as unknown as" assertion to access startRtkHealthCheck on presenter.newAgentPresenter; replace this with a proper typed approach by either (a) adding an optional method to the presenter/new-agent presenter interface (e.g., extend the presenter type to include startRtkHealthCheck?: () => Promise<void>) and cast to that interface, or (b) implement a small type guard function (e.g., isNewAgentPresenterWithHealthCheck(obj): obj is { startRtkHealthCheck: () => Promise<void> }) that checks typeof obj?.startRtkHealthCheck === 'function' and use that guard to verify presenter.newAgentPresenter before calling startRtkHealthCheck; update the code to use the chosen interface or guard instead of "as unknown as" and keep the existing early return if the method is absent.src/main/lib/agentRuntime/rtkRuntimeService.ts (1)
647-668: Consider async mkdir in async method.
createRuntimeEnvis an async method but uses synchronousfs.mkdirSyncon Line 663. While this typically executes quickly, it could block the event loop if the filesystem is slow.♻️ Optional: Use async mkdir
- if (dbPath) { - fs.mkdirSync(path.dirname(dbPath), { recursive: true }) - env.RTK_DB_PATH = dbPath - } + if (dbPath) { + await fs.promises.mkdir(path.dirname(dbPath), { recursive: true }) + env.RTK_DB_PATH = dbPath + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/rtkRuntimeService.ts` around lines 647 - 668, The createRuntimeEnv async function uses blocking fs.mkdirSync when dbPath is provided; replace the synchronous call with the async variant (e.g., await fs.promises.mkdir(path.dirname(dbPath), { recursive: true }) or await fs.mkdir(path.dirname(dbPath), { recursive: true })) before setting env.RTK_DB_PATH, ensuring the method remains async and does not block the event loop; update imports/usages accordingly and preserve the existing behavior when dbPath is undefined.src/main/presenter/agentPresenter/acp/agentBashHandler.ts (2)
457-469: Background execution lacks RTK fallback recovery.Unlike foreground execution, background commands cannot automatically fall back to the original command if RTK rewriting causes a failure. If RTK rewrites a command incorrectly for background execution, it will fail without recovery since the method returns immediately with a session ID.
Consider documenting this limitation or implementing a mechanism where
backgroundExecSessionManagercould report RTK failures that could be surfaced to the user for manual retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts` around lines 457 - 469, Background backgroundExec lacks RTK fallback: prepareCommand may rewrite the command with RTK and backgroundExecSessionManager.start returns immediately so RTK rewrite failures aren't surfaced or retried. Modify the flow around prepareCommand and backgroundExecSessionManager.start to detect RTK failure indicators (rtkApplied/rtkFallbackReason) returned by prepareCommand and either (a) perform a synchronous validation step before starting the background session and, on validation failure, fall back to the original command and update the returned output accordingly, or (b) extend backgroundExecSessionManager.start to accept and report an RTK preflight result/error so the caller can include rtkFallbackReason/status in the returned output; update the returned object (output.status/sessionId, rtkApplied, rtkMode, rtkFallbackReason) to reflect any fallback or validation failure so users can surface retry instructions.
505-517: Extract error patterns as constants for maintainability.The fallback detection uses hardcoded error message substrings. While the test cases confirm these patterns accurately reflect RTK runtime errors, extracting them as SCREAMING_SNAKE_CASE constants would:
- Reduce duplication between tests and implementation
- Make patterns easier to maintain if RTK changes error messages
- Clarify which error conditions trigger fallbacks
Consider defining constants like
RTK_COMPOUND_PREDICATES_ERROR,RTK_UNSUPPORTED_PREDICATE_ERROR, andRTK_UNSUPPORTED_ACTION_ERRORat the top of the file, then use them in both the method and test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts` around lines 505 - 517, Extract the hardcoded RTK error substrings in getRtkCapabilityFallbackReason into SCREAMING_SNAKE_CASE constants (e.g., RTK_COMPOUND_PREDICATES_ERROR, RTK_UNSUPPORTED_PREDICATE_ERROR, RTK_UNSUPPORTED_ACTION_ERROR) declared near the top of the file and replace the literal strings in getRtkCapabilityFallbackReason with those constants; update tests to import/use the same constants so patterns are centralized and maintainable while preserving the existing includes() checks and return messages from getRtkCapabilityFallbackReason.
🤖 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/lib/runtimeHelper.ts`:
- Around line 401-427: The code only triggers the bundled runtime replacement
when basename === 'rtk', so Windows calls like 'rtk.exe' are missed; update the
conditional that checks basename (used around this.rtkRuntimePath, checkExists,
and command) to also match 'rtk.exe' on Windows—either normalize basename by
lowercasing and stripping a '.exe' extension before comparison, or change the
check to if (basename === 'rtk' || (process.platform === 'win32' && basename ===
'rtk.exe'))—then the existing logic that builds rtkPath and uses fs.existsSync
can remain unchanged.
In `@src/main/presenter/filePresenter/mime.ts`:
- Around line 116-118: Normalize the file extension before checking
TYPESCRIPT_EXTENSIONS: compute a lowercase extension from path.extname(filePath)
(e.g., ext = path.extname(filePath).toLowerCase()) and use that in the
conditional that currently reads
TYPESCRIPT_EXTENSIONS.has(path.extname(filePath)); then call
isLikelyTextFile(filePath) unchanged and return 'application/typescript' when
appropriate; also ensure TYPESCRIPT_EXTENSIONS contains lowercase entries to
match the normalized ext.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 207-235: The rtk localization block (keys like "rtk.title",
"rtk.description", "rtk.actions.retry", all "rtk.status.*",
"rtk.descriptionDisabled", "rtk.descriptionChecking", "rtk.descriptionHealthy",
"rtk.descriptionUnhealthy", "rtk.sourceLabel", "rtk.source.bundled|system|none",
and all "rtk.summary.*") is still in English in the da-DK file; replace each
English string with the correct Danish translations so the Danish locale
contains fully localized values for the entire rtk section.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 261-289: The RTK block currently contains English strings; replace
each value under the "rtk" object (including "title", "description",
"actions.retry", all "status" values, "descriptionDisabled",
"descriptionChecking", "descriptionHealthy", "descriptionUnhealthy",
"sourceLabel", "source.bundled", "source.system", "source.none", and every key
under "summary" like "savedTokens", "commands", "avgSavingsPct", "outputTokens")
with appropriate Persian translations consistent with the rest of the fa-IR
file, preserving interpolation tokens such as {source} and keeping the JSON keys
unchanged.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 261-289: The JSON "rtk" block contains English-only strings
causing mixed-language UX; translate every value under the "rtk" object into
French (keys to update include "title", "description", "actions.retry",
"status.disabled|checking|healthy|unhealthy",
"descriptionDisabled|descriptionChecking|descriptionHealthy|descriptionUnhealthy",
"sourceLabel", "source.bundled|source.system|source.none", and
"summary.savedTokens|commands|avgSavingsPct|outputTokens") so the fr-FR locale
is fully localized; replace each English phrase with the appropriate French
translation while preserving the JSON keys and interpolation token "{source}" in
"sourceLabel".
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 261-289: The RTK block (key "rtk") currently contains English
strings; replace each value with Hebrew translations while preserving
placeholders and key structure—specifically translate "title", "description",
"actions.retry", all "status" values
("disabled","checking","healthy","unhealthy"), the descriptive keys
("descriptionDisabled","descriptionChecking","descriptionHealthy","descriptionUnhealthy"),
"sourceLabel" (keep the "{source}" placeholder), "source" subkeys
("bundled","system","none"), and all "summary" labels
("savedTokens","commands","avgSavingsPct","outputTokens") so the RTK section
matches the rest of the he-IL localization.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 261-289: The new "rtk" localization block in ja-JP is still in
English; translate all keys under rtk (title, description, actions.retry,
status.disabled/checking/healthy/unhealthy,
descriptionDisabled/descriptionChecking/descriptionHealthy/descriptionUnhealthy,
sourceLabel, source.bundled/system/none, and
summary.savedTokens/commands/avgSavingsPct/outputTokens) into natural Japanese,
preserving the key names and any placeholders like {source}; update
src/renderer/src/i18n/ja-JP/settings.json so the UI shows Japanese text for
these rtk entries.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Around line 261-289: The RTK section in the Korean locale contains English
strings; update the values under the "rtk" JSON object (keys: title,
description, actions.retry, status.disabled/checking/healthy/unhealthy,
descriptionDisabled/descriptionChecking/descriptionHealthy/descriptionUnhealthy,
sourceLabel, source.bundled/system/none, and
summary.savedTokens/commands/avgSavingsPct/outputTokens) with proper Korean
translations so the ko-KR file is fully localized; keep the keys unchanged and
only replace the English string values with their Korean equivalents, preserving
placeholders like "{source}" in sourceLabel.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 261-289: The rtk localization block is still in English; update
the pt-BR settings.json "rtk" object by translating each key's value (title,
description, actions.retry, status.disabled/checking/healthy/unhealthy,
descriptionDisabled/Checking/Healthy/Unhealthy, sourceLabel,
source.bundled/system/none, and
summary.savedTokens/commands/avgSavingsPct/outputTokens) into Brazilian
Portuguese so the dashboard.rtk UI shows Portuguese strings for pt-BR users
(locate the "rtk" object and replace the English phrases with appropriate
Portuguese equivalents).
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 261-289: The RTK localization block under the "rtk" key is still
in English; translate all string values (e.g., "title", "description",
"actions.retry", "status.disabled/checking/healthy/unhealthy",
"descriptionDisabled/Checking/Healthy/Unhealthy", "sourceLabel",
"source.bundled/system/none", and
"summary.savedTokens/commands/avgSavingsPct/outputTokens") into Russian so the
ru-RU settings.json provides full Russian UI text; keep placeholders like
{source} intact and preserve key names exactly.
---
Nitpick comments:
In `@package.json`:
- Around line 47-53: The package.json added provisioning for runtime/rtk and
runtime/ripgrep in scripts like "installRuntime" and the platform variants
("installRuntime:win:x64", "installRuntime:mac:arm64", etc.), but the cleanup
scripts (e.g., "cleanRuntime" and any platform-specific clean scripts) were not
updated; modify the clean scripts to remove ./runtime/rtk and ./runtime/ripgrep
(add rm -rf ./runtime/rtk and rm -rf ./runtime/ripgrep alongside the existing
removals) so all install and clean script sets are symmetrical.
In `@src/main/lib/agentRuntime/rtkRuntimeService.ts`:
- Around line 647-668: The createRuntimeEnv async function uses blocking
fs.mkdirSync when dbPath is provided; replace the synchronous call with the
async variant (e.g., await fs.promises.mkdir(path.dirname(dbPath), { recursive:
true }) or await fs.mkdir(path.dirname(dbPath), { recursive: true })) before
setting env.RTK_DB_PATH, ensuring the method remains async and does not block
the event loop; update imports/usages accordingly and preserve the existing
behavior when dbPath is undefined.
In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts`:
- Around line 457-469: Background backgroundExec lacks RTK fallback:
prepareCommand may rewrite the command with RTK and
backgroundExecSessionManager.start returns immediately so RTK rewrite failures
aren't surfaced or retried. Modify the flow around prepareCommand and
backgroundExecSessionManager.start to detect RTK failure indicators
(rtkApplied/rtkFallbackReason) returned by prepareCommand and either (a) perform
a synchronous validation step before starting the background session and, on
validation failure, fall back to the original command and update the returned
output accordingly, or (b) extend backgroundExecSessionManager.start to accept
and report an RTK preflight result/error so the caller can include
rtkFallbackReason/status in the returned output; update the returned object
(output.status/sessionId, rtkApplied, rtkMode, rtkFallbackReason) to reflect any
fallback or validation failure so users can surface retry instructions.
- Around line 505-517: Extract the hardcoded RTK error substrings in
getRtkCapabilityFallbackReason into SCREAMING_SNAKE_CASE constants (e.g.,
RTK_COMPOUND_PREDICATES_ERROR, RTK_UNSUPPORTED_PREDICATE_ERROR,
RTK_UNSUPPORTED_ACTION_ERROR) declared near the top of the file and replace the
literal strings in getRtkCapabilityFallbackReason with those constants; update
tests to import/use the same constants so patterns are centralized and
maintainable while preserving the existing includes() checks and return messages
from getRtkCapabilityFallbackReason.
In
`@src/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.ts`:
- Around line 15-20: The code uses an unsafe "as unknown as" assertion to access
startRtkHealthCheck on presenter.newAgentPresenter; replace this with a proper
typed approach by either (a) adding an optional method to the
presenter/new-agent presenter interface (e.g., extend the presenter type to
include startRtkHealthCheck?: () => Promise<void>) and cast to that interface,
or (b) implement a small type guard function (e.g.,
isNewAgentPresenterWithHealthCheck(obj): obj is { startRtkHealthCheck: () =>
Promise<void> }) that checks typeof obj?.startRtkHealthCheck === 'function' and
use that guard to verify presenter.newAgentPresenter before calling
startRtkHealthCheck; update the code to use the chosen interface or guard
instead of "as unknown as" and keep the existing early return if the method is
absent.
In `@src/renderer/src/components/message/MessageBlockToolCall.vue`:
- Around line 17-23: The RTK badge text in MessageBlockToolCall.vue is hardcoded
in the span (v-if="showRtkBadge" with data-testid="tool-call-rtk-badge");
replace the literal "RTK" with a vue-i18n lookup (e.g. use
$t('toolCall.rtkBadge') or the composition API equivalent) and add the key
"toolCall.rtkBadge": "RTK" to the project's i18n JSON files so the UI text comes
from localization resources; keep the existing data-testid and class attributes
unchanged.
In `@src/renderer/src/i18n/zh-TW/settings.json`:
- Around line 279-285: Update the Traditional Chinese translations in the
settings JSON: change the value for the "bundled" key from "內置" to "內建" to match
the "builtIn" terminology used elsewhere, and change the value for the
"commands" key from "已跟蹤命令" to "已追蹤命令" to use the Taiwan-preferred term; make
these edits in the same object that contains "bundled" and "summary" so the file
remains consistent with the "builtIn": "內建" usage.
In `@test/main/lib/agentRuntime/rtkRuntimeService.test.ts`:
- Around line 26-36: Tests are mutating private fields via "as never"
(healthState, resolvedRuntime) on RtkRuntimeService; add a package-private test
factory on RtkRuntimeService (e.g., static _createWithState(deps:
RtkRuntimeServiceDeps, healthState: HealthState, resolvedRuntime:
ResolvedRuntime): RtkRuntimeService) that constructs an instance and sets those
internal fields, update tests to use RtkRuntimeService._createWithState(...)
instead of casting to never, and keep the factory marked/test-only so production
API is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed924a5c-d7bd-43ba-9d63-458def2ceebd
📒 Files selected for processing (42)
package.jsonsrc/main/lib/agentRuntime/rtkRuntimeService.tssrc/main/lib/runtimeHelper.tssrc/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/filePresenter/mime.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.tssrc/main/presenter/lifecyclePresenter/hooks/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/toolPresenter/index.tssrc/renderer/settings/components/DashboardSettings.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat.tssrc/shared/types/core/mcp.tssrc/shared/types/presenters/new-agent.presenter.d.tssrc/shared/types/presenters/tool.presenter.d.tstest/main/lib/agentRuntime/rtkRuntimeService.test.tstest/main/presenter/agentPresenter/acp/agentBashHandler.test.tstest/main/presenter/filePresenter/mime.test.tstest/main/presenter/newAgentPresenter/newAgentPresenter.test.tstest/main/presenter/newAgentPresenter/usageDashboard.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/presenter/toolPresenter/toolPresenter.test.tstest/renderer/components/DashboardSettings.test.tstest/renderer/components/message/MessageBlockToolCall.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main/lib/runtimeHelper.test.ts (1)
27-43: Add a same-test assertion for plainrtkto cover both normalized forms.You already validate
rtk.exe; addingrtkin the same test will guard the normalization branch end-to-end.✅ Suggested test addition
expect(helper.replaceWithRuntimeCommand('rtk.exe', true, true)).toBe( '/mock/runtime/rtk/rtk.exe' ) + expect(helper.replaceWithRuntimeCommand('rtk', true, true)).toBe( + '/mock/runtime/rtk/rtk.exe' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/lib/runtimeHelper.test.ts` around lines 27 - 43, The test currently asserts replacement for 'rtk.exe' only; add an additional assertion in the same test to call RuntimeHelper.getInstance().replaceWithRuntimeCommand('rtk', true, true) (after setting (helper as never).rtkRuntimePath and the fs.existsSync mock) and expect it to return '/mock/runtime/rtk/rtk.exe' so the normalization branch for plain 'rtk' is exercised end-to-end; keep the platform override and mocks as-is and place the new expect directly after the existing one.
🤖 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/lib/runtimeHelper.ts`:
- Around line 107-123: The new const identifiers don't follow the project's
SCREAMING_SNAKE_CASE rule; rename the constants used in runtime detection (e.g.,
rtkRuntimePath, rtkExe, rtkBin and the other consts mentioned like candidates
and existingPath) to SCREAMING_SNAKE_CASE (e.g., RTK_RUNTIME_PATH, RTK_EXE,
RTK_BIN, CANDIDATES, EXISTING_PATH) and update all references in the same
function/method (runtime detection logic in runtimeHelper.ts) so usages match
the new names.
---
Nitpick comments:
In `@test/main/lib/runtimeHelper.test.ts`:
- Around line 27-43: The test currently asserts replacement for 'rtk.exe' only;
add an additional assertion in the same test to call
RuntimeHelper.getInstance().replaceWithRuntimeCommand('rtk', true, true) (after
setting (helper as never).rtkRuntimePath and the fs.existsSync mock) and expect
it to return '/mock/runtime/rtk/rtk.exe' so the normalization branch for plain
'rtk' is exercised end-to-end; keep the platform override and mocks as-is and
place the new expect directly after the existing one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b4fc541-92af-4cb7-8c71-1b4ba1dce92f
📒 Files selected for processing (12)
src/main/lib/runtimeHelper.tssrc/main/presenter/filePresenter/mime.tssrc/renderer/src/i18n/da-DK/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.jsontest/main/lib/runtimeHelper.test.tstest/main/presenter/filePresenter/mime.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/src/i18n/ru-RU/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- test/main/presenter/filePresenter/mime.test.ts
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
Comment on lines
+107
to
+123
| // Check if RTK runtime file exists | ||
| const rtkRuntimePath = path.join(runtimeBasePath, 'rtk') | ||
| if (process.platform === 'win32') { | ||
| const rtkExe = path.join(rtkRuntimePath, 'rtk.exe') | ||
| if (fs.existsSync(rtkExe)) { | ||
| this.rtkRuntimePath = rtkRuntimePath | ||
| } else { | ||
| this.rtkRuntimePath = null | ||
| } | ||
| } else { | ||
| const rtkBin = path.join(rtkRuntimePath, 'rtk') | ||
| if (fs.existsSync(rtkBin)) { | ||
| this.rtkRuntimePath = rtkRuntimePath | ||
| } else { | ||
| this.rtkRuntimePath = null | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Rename newly added const declarations to SCREAMING_SNAKE_CASE.
The new constants in these blocks (e.g., rtkRuntimePath, rtkExe, candidates, existingPath) don’t follow the repository’s constant naming rule for src/**/*.ts.
As per coding guidelines, "Use SCREAMING_SNAKE_CASE for constant declarations".
Also applies to: 164-225
🤖 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 107 - 123, The new const
identifiers don't follow the project's SCREAMING_SNAKE_CASE rule; rename the
constants used in runtime detection (e.g., rtkRuntimePath, rtkExe, rtkBin and
the other consts mentioned like candidates and existingPath) to
SCREAMING_SNAKE_CASE (e.g., RTK_RUNTIME_PATH, RTK_EXE, RTK_BIN, CANDIDATES,
EXISTING_PATH) and update all references in the same function/method (runtime
detection logic in runtimeHelper.ts) so usages match the new names.
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 (1)
src/main/presenter/floatingButtonPresenter/index.ts (1)
491-526:⚠️ Potential issue | 🟡 MinorAdd i18n support for context menu labels.
The context menu labels "打开主窗口" and "退出应用" are hardcoded. Use
getContextMenuLabelsfrom@shared/i18nfor consistency with other presenters liketrayPresenter.tsandtabPresenter.ts, which follow the pattern of getting the current locale and retrieving translated labels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/floatingButtonPresenter/index.ts` around lines 491 - 526, The showContextMenu method currently uses hardcoded Chinese labels; replace them by importing and calling getContextMenuLabels from `@shared/i18n` (same pattern as trayPresenter/tabPresenter) and use the returned labels when building the template in showContextMenu; keep the existing Menu.buildFromTemplate usage and popup logic (referencing floatingWindow, getWindow, presenter.windowPresenter.mainWindow) but construct template entries with the i18n-provided strings for the '打开主窗口' and '退出应用' labels.
🧹 Nitpick comments (8)
src/renderer/src/events.ts (1)
189-204: Move the floating-button event contract intosrc/shared.Lines 193-204 grow
FLOATING_BUTTON_EVENTSinto a main/preload/renderer protocol, but the constants still live insrc/renderer/src. That makes drift between process-specific copies much more likely as the surface keeps expanding.As per coding guidelines, "Organize shared TypeScript types and utilities in
src/shared/for use across main, preload, and renderer processes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/events.ts` around lines 189 - 204, The floating-button event contract (FLOATING_BUTTON_EVENTS) is defined in src/renderer/src/events.ts but must live in the shared layer so main/preload/renderer use a single source of truth; move the FLOATING_BUTTON_EVENTS constant (the keys like RIGHT_CLICKED, VISIBILITY_CHANGED, POSITION_CHANGED, etc.) into a new or existing module under src/shared (e.g., src/shared/floatingButtonEvents.ts), export it from there, then update src/renderer/src/events.ts and any main/preload modules to import FLOATING_BUTTON_EVENTS from src/shared instead of keeping process-specific copies.src/main/presenter/configPresenter/index.ts (1)
767-771: Centralize floating-widget refreshes and verifyrefreshTheme()handling.Lines 768, 1487, and 1502 now reach directly into
presenter.floatingButtonPresenter, which spreads this config-driven refresh path acrossConfigPresenterinstead of keeping it behind one owner. Also,refreshTheme()is awaited insrc/main/presenter/floatingButtonPresenter/index.ts, so if it returns a promise the synchronoustry/catchblocks on Lines 1486-1490 and 1501-1504 still will not catch rejected refreshes.As per coding guidelines, "Use Presenter pattern in main process with presenters organized in
presenter/subdirectory (Window/Tab/Thread/Mcp/Config/LLMProvider); maintain centralized event handling ineventbus.tsfor app events".Also applies to: 1486-1490, 1501-1504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 767 - 771, Callers are reaching directly into presenter.floatingButtonPresenter and using synchronous try/catch around refreshTheme(), which won't catch rejected promises; add a single async method on ConfigPresenter (e.g., ConfigPresenter.refreshFloatingWidget) that encapsulates presenter.floatingButtonPresenter.refreshLanguage() and awaits presenter.floatingButtonPresenter.refreshTheme() inside its own try/catch, then replace direct accesses with awaited calls to ConfigPresenter.refreshFloatingWidget so all refreshes are centralized and promise rejections are handled.test/renderer/stores/sessionStore.test.ts (1)
231-240: Add the other-renderer guard case.Line 234 includes
webContentsId, so the branch most likely to regress here is “ignore activation events for a different renderer.” The happy-path assertion alone will still pass if cross-window activation starts hijacking unrelated tabs.Follow-up test
+ it('ignores external session activation for a different renderer', async () => { + const { store, pageRouter, emitIpc, SESSION_EVENTS } = await setupStore() + + emitIpc(SESSION_EVENTS.ACTIVATED, { + webContentsId: 999, + sessionId: 'session-other-renderer' + }) + + expect(store.activeSessionId.value).not.toBe('session-other-renderer') + expect(pageRouter.goToChat).not.toHaveBeenCalled() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/stores/sessionStore.test.ts` around lines 231 - 240, The test only covers the happy path for SESSION_EVENTS.ACTIVATED but misses the guard for activations from other renderers; add a test (or extend this one) using setupStore()/emitIpc() to emit SESSION_EVENTS.ACTIVATED with a webContentsId that does NOT match this renderer's id and assert that store.activeSessionId.value remains unchanged and pageRouter.goToChat was not called; reference SESSION_EVENTS.ACTIVATED, emitIpc, setupStore, store.activeSessionId, and pageRouter.goToChat to locate and implement the ignored-other-renderer case.src/main/events.ts (1)
228-236: Avoid cross-process event literal drift by centralizing floating event constants.These new keys are fine, but
FLOATING_BUTTON_EVENTSis duplicated in preload-side code. Consider moving floating event names to a shared module consumable by main/preload/renderer to prevent silent desync in future changes.As per coding guidelines:
src/shared/**/*.ts: Organize shared TypeScript types and utilities insrc/shared/for use across main, preload, and renderer processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/events.ts` around lines 228 - 236, The floating event literals defined in FLOATING_BUTTON_EVENTS should be centralized into a shared module to prevent drift; create a new shared constant (e.g., export const FLOATING_BUTTON_EVENTS = { ... }) in the shared utilities (src/shared) and replace the local definition in main (the current FLOATING_BUTTON_EVENTS) and the duplicate in preload with imports from that shared module; update all usages (e.g., references in main event registration, preload IPC code, and renderer consumers) to import the single source-of-truth and remove the duplicated literal definitions so all processes use the same exported symbol.src/renderer/floating/components/FloatingSessionItem.vue (1)
20-78: Consider consolidating status-derived UI config into one map.Line 20 through Line 78 repeats status switching across multiple computed values. A single status config object (label key + class tokens) would reduce drift risk and simplify future status additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/floating/components/FloatingSessionItem.vue` around lines 20 - 78, Multiple computed properties (statusLabel, statusClass, itemClass, accentClass, dotClass) all switch on props.session.status, causing duplicated logic; replace them with a single statusConfig map keyed by status values (e.g., 'in_progress', 'error', 'done') that contains label, statusClass, itemClass, accentClass, and dotClass entries, then have each computed read the appropriate field from statusConfig[props.session.status] with a sensible default; update references in the component so statusLabel, statusClass, itemClass, accentClass, and dotClass simply return the corresponding values from that unified config.src/renderer/floating/FloatingButton.vue (1)
177-186: Consider adding a guard for missingfloatingButtonAPI.If the preload script fails to expose
floatingButtonAPI, accessingwindow.floatingButtonAPIwill throw. Adding a defensive check would improve resilience.🛡️ Proposed defensive guard
onMounted(async () => { + if (!window.floatingButtonAPI) { + console.error('floatingButtonAPI not available') + return + } + try { snapshot.value = await window.floatingButtonAPI.getSnapshot() } catch (error) { console.warn('Failed to initialize floating widget snapshot:', error) } window.floatingButtonAPI.onSnapshotUpdate(handleSnapshotUpdate) window.addEventListener('blur', handleWindowBlur) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/floating/FloatingButton.vue` around lines 177 - 186, The mount routine currently assumes window.floatingButtonAPI exists and will throw if it's missing; update the onMounted handler to check for window.floatingButtonAPI before calling getSnapshot, onSnapshotUpdate, or any methods and only add the blur listener / call snapshot.value when the API is present (use a guard like if (!window.floatingButtonAPI) return or conditional branches). Specifically protect calls to getSnapshot, onSnapshotUpdate (and keep using handleSnapshotUpdate), and avoid registering those listeners while still allowing handleWindowBlur registration only if intended; ensure snapshot.value is only assigned after a successful getSnapshot call.src/main/presenter/floatingButtonPresenter/layout.ts (2)
29-39: Status mapping doesn't explicitly handle allSessionStatusvalues.The
SessionStatustype includes'paused','waiting_permission', and'waiting_question'(per context snippet fromsession.presenter.d.ts), but these fall through to the default'done'case. This may be intentional, but explicit handling would make the mapping clearer and catch future status additions.💡 Explicit status mapping
function mapSessionStatus(status: SessionWithState['status']): FloatingWidgetSessionStatus { switch (status) { case 'generating': return 'in_progress' case 'error': return 'error' + case 'paused': + case 'waiting_permission': + case 'waiting_question': case 'idle': - default: return 'done' + default: { + // Exhaustive check - will cause compile error if new status is added + const _exhaustive: never = status + return 'done' + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/floatingButtonPresenter/layout.ts` around lines 29 - 39, mapSessionStatus currently falls through non-explicit SessionStatus values to the default 'done', which is unclear and brittle; update the mapSessionStatus function to explicitly handle 'paused', 'waiting_permission', and 'waiting_question' (using the intended mappings to FloatingWidgetSessionStatus) by adding case arms for each of those values and return the correct FloatingWidgetSessionStatus, and replace the broad default with an explicit default that either returns a safe fallback or throws/asserts for unexpected statuses (referencing mapSessionStatus, SessionWithState['status'], and FloatingWidgetSessionStatus to locate where to change).
92-96:collapsedIdleandcollapsedBusyhave identical dimensions.Both collapsed states currently use the same 64x64 dimensions. If this is intentional (same size regardless of activity), consider using a single
collapsedconstant to reduce duplication.💡 Simplification if sizes should remain identical
export const FLOATING_WIDGET_LAYOUT = { - collapsedIdle: { width: 64, height: 64 }, - collapsedBusy: { width: 64, height: 64 }, + collapsed: { width: 64, height: 64 }, expandedWidth: 388, // ... } as constThen update
getCollapsedWidgetSizeto always returnFLOATING_WIDGET_LAYOUT.collapsed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/floatingButtonPresenter/layout.ts` around lines 92 - 96, getCollapsedWidgetSize currently picks between FLOATING_WIDGET_LAYOUT.collapsedBusy and collapsedIdle which are identical; consolidate to one constant to remove duplication: add FLOATING_WIDGET_LAYOUT.collapsed (64x64) and update getCollapsedWidgetSize to always return { ...FLOATING_WIDGET_LAYOUT.collapsed } (or, if you must keep the two names for semantic reasons, set both collapsedIdle and collapsedBusy to reference FLOATING_WIDGET_LAYOUT.collapsed). Update any other usages to use the new collapsed constant and remove redundant size literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 2576-2580: The current try/catch around
presenter.floatingButtonPresenter.refreshWidgetState() cannot catch rejections
because refreshWidgetState() returns a Promise and is being suppressed with
void; update the call to either await
presenter.floatingButtonPresenter.refreshWidgetState() inside an async function
or call presenter.floatingButtonPresenter.refreshWidgetState().catch(error =>
processLogger.warn(...)) so rejections are handled explicitly, and additionally
prefer emitting a centralized event (e.g., via
eventBus.emit('FloatingWidgetRefresh')) instead of directly coupling presenters
so the floating-button presenter listens and reacts to the event; apply the same
fix pattern where presenter's async calls are suppressed (notably
presenter.floatingButtonPresenter.refreshWidgetState and the analogous call in
newAgentPresenter).
In `@src/main/presenter/floatingButtonPresenter/index.ts`:
- Around line 484-488: Replace the fragile type-assertion access to the internal
Map by adding a stable public accessor on windowPresenter (e.g., add a method
getWindowById(id: number): BrowserWindow | null on the windowPresenter
implementation) and call that method instead of using (windowPresenter as
...).windows.get(createdWindowId); update the call site to use
windowPresenter.getWindowById(createdWindowId) and remove the type assertion/Map
access to avoid relying on private internals.
In `@src/preload/floating-preload.ts`:
- Around line 91-107: The three methods onSnapshotUpdate, onLanguageChanged, and
onThemeChanged add persistent ipcRenderer listeners each call and can accumulate
duplicates; modify each to first remove any previous listener for the same
FLOATING_BUTTON_EVENTS event (or use ipcRenderer.once) and return an unsubscribe
function so callers can clean up when the renderer unmounts. Specifically update
the implementations that register FLOATING_BUTTON_EVENTS.SNAPSHOT_UPDATED,
.LANGUAGE_CHANGED, and .THEME_CHANGED to either (a) call
ipcRenderer.removeListener for the same callback before adding, or (b) use a
wrapped one-time listener and provide a cleanup function that calls
ipcRenderer.removeListener to unregister the handler.
In `@src/renderer/floating/FloatingButton.vue`:
- Around line 229-233: Replace the relative asset import with the project alias
in FloatingButton.vue: change the img src attributes that currently use
"../src/assets/logo.png" (the <img> element with class "logo-orb-image
status-orb-logo-image h-9 w-9" and the other instance later in the file) to use
"@/assets/logo.png" so they match the rest of the codebase's asset imports.
---
Outside diff comments:
In `@src/main/presenter/floatingButtonPresenter/index.ts`:
- Around line 491-526: The showContextMenu method currently uses hardcoded
Chinese labels; replace them by importing and calling getContextMenuLabels from
`@shared/i18n` (same pattern as trayPresenter/tabPresenter) and use the returned
labels when building the template in showContextMenu; keep the existing
Menu.buildFromTemplate usage and popup logic (referencing floatingWindow,
getWindow, presenter.windowPresenter.mainWindow) but construct template entries
with the i18n-provided strings for the '打开主窗口' and '退出应用' labels.
---
Nitpick comments:
In `@src/main/events.ts`:
- Around line 228-236: The floating event literals defined in
FLOATING_BUTTON_EVENTS should be centralized into a shared module to prevent
drift; create a new shared constant (e.g., export const FLOATING_BUTTON_EVENTS =
{ ... }) in the shared utilities (src/shared) and replace the local definition
in main (the current FLOATING_BUTTON_EVENTS) and the duplicate in preload with
imports from that shared module; update all usages (e.g., references in main
event registration, preload IPC code, and renderer consumers) to import the
single source-of-truth and remove the duplicated literal definitions so all
processes use the same exported symbol.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 767-771: Callers are reaching directly into
presenter.floatingButtonPresenter and using synchronous try/catch around
refreshTheme(), which won't catch rejected promises; add a single async method
on ConfigPresenter (e.g., ConfigPresenter.refreshFloatingWidget) that
encapsulates presenter.floatingButtonPresenter.refreshLanguage() and awaits
presenter.floatingButtonPresenter.refreshTheme() inside its own try/catch, then
replace direct accesses with awaited calls to
ConfigPresenter.refreshFloatingWidget so all refreshes are centralized and
promise rejections are handled.
In `@src/main/presenter/floatingButtonPresenter/layout.ts`:
- Around line 29-39: mapSessionStatus currently falls through non-explicit
SessionStatus values to the default 'done', which is unclear and brittle; update
the mapSessionStatus function to explicitly handle 'paused',
'waiting_permission', and 'waiting_question' (using the intended mappings to
FloatingWidgetSessionStatus) by adding case arms for each of those values and
return the correct FloatingWidgetSessionStatus, and replace the broad default
with an explicit default that either returns a safe fallback or throws/asserts
for unexpected statuses (referencing mapSessionStatus,
SessionWithState['status'], and FloatingWidgetSessionStatus to locate where to
change).
- Around line 92-96: getCollapsedWidgetSize currently picks between
FLOATING_WIDGET_LAYOUT.collapsedBusy and collapsedIdle which are identical;
consolidate to one constant to remove duplication: add
FLOATING_WIDGET_LAYOUT.collapsed (64x64) and update getCollapsedWidgetSize to
always return { ...FLOATING_WIDGET_LAYOUT.collapsed } (or, if you must keep the
two names for semantic reasons, set both collapsedIdle and collapsedBusy to
reference FLOATING_WIDGET_LAYOUT.collapsed). Update any other usages to use the
new collapsed constant and remove redundant size literals.
In `@src/renderer/floating/components/FloatingSessionItem.vue`:
- Around line 20-78: Multiple computed properties (statusLabel, statusClass,
itemClass, accentClass, dotClass) all switch on props.session.status, causing
duplicated logic; replace them with a single statusConfig map keyed by status
values (e.g., 'in_progress', 'error', 'done') that contains label, statusClass,
itemClass, accentClass, and dotClass entries, then have each computed read the
appropriate field from statusConfig[props.session.status] with a sensible
default; update references in the component so statusLabel, statusClass,
itemClass, accentClass, and dotClass simply return the corresponding values from
that unified config.
In `@src/renderer/floating/FloatingButton.vue`:
- Around line 177-186: The mount routine currently assumes
window.floatingButtonAPI exists and will throw if it's missing; update the
onMounted handler to check for window.floatingButtonAPI before calling
getSnapshot, onSnapshotUpdate, or any methods and only add the blur listener /
call snapshot.value when the API is present (use a guard like if
(!window.floatingButtonAPI) return or conditional branches). Specifically
protect calls to getSnapshot, onSnapshotUpdate (and keep using
handleSnapshotUpdate), and avoid registering those listeners while still
allowing handleWindowBlur registration only if intended; ensure snapshot.value
is only assigned after a successful getSnapshot call.
In `@src/renderer/src/events.ts`:
- Around line 189-204: The floating-button event contract
(FLOATING_BUTTON_EVENTS) is defined in src/renderer/src/events.ts but must live
in the shared layer so main/preload/renderer use a single source of truth; move
the FLOATING_BUTTON_EVENTS constant (the keys like RIGHT_CLICKED,
VISIBILITY_CHANGED, POSITION_CHANGED, etc.) into a new or existing module under
src/shared (e.g., src/shared/floatingButtonEvents.ts), export it from there,
then update src/renderer/src/events.ts and any main/preload modules to import
FLOATING_BUTTON_EVENTS from src/shared instead of keeping process-specific
copies.
In `@test/renderer/stores/sessionStore.test.ts`:
- Around line 231-240: The test only covers the happy path for
SESSION_EVENTS.ACTIVATED but misses the guard for activations from other
renderers; add a test (or extend this one) using setupStore()/emitIpc() to emit
SESSION_EVENTS.ACTIVATED with a webContentsId that does NOT match this
renderer's id and assert that store.activeSessionId.value remains unchanged and
pageRouter.goToChat was not called; reference SESSION_EVENTS.ACTIVATED, emitIpc,
setupStore, store.activeSessionId, and pageRouter.goToChat to locate and
implement the ignored-other-renderer case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f77b69c2-e356-4e0b-8583-5f62fc10869d
📒 Files selected for processing (34)
docs/specs/floating-agent-widget/plan.mddocs/specs/floating-agent-widget/spec.mddocs/specs/floating-agent-widget/tasks.mdsrc/main/events.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/floatingButtonPresenter/FloatingButtonWindow.tssrc/main/presenter/floatingButtonPresenter/index.tssrc/main/presenter/floatingButtonPresenter/layout.tssrc/main/presenter/floatingButtonPresenter/types.tssrc/main/presenter/newAgentPresenter/index.tssrc/preload/floating-preload.tssrc/renderer/floating/FloatingButton.vuesrc/renderer/floating/components/FloatingSessionItem.vuesrc/renderer/floating/env.d.tssrc/renderer/floating/main.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/stores/ui/session.tssrc/shared/featureFlags.tssrc/shared/types/floating-widget.tstest/main/presenter/floatingButtonPresenter/layout.test.tstest/renderer/stores/sessionStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/newAgentPresenter/index.ts
Comment on lines
+2576
to
+2580
| try { | ||
| void presenter.floatingButtonPresenter.refreshWidgetState() | ||
| } catch (error) { | ||
| console.warn('[DeepChatAgent] Failed to refresh floating widget state:', error) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether refreshWidgetState is Promise/async and how it's called.
rg -n --type ts -C3 '\brefreshWidgetState\s*\('
rg -n --type ts -C3 'async\s+refreshWidgetState\s*\('
rg -n --type ts -C3 'void\s+.*refreshWidgetState\s*\('Repository: ThinkInAIXYZ/deepchat
Length of output: 5553
Handle unhandled Promise rejections explicitly; try-catch only guards synchronous throws.
refreshWidgetState() is an async method returning Promise<void>. The current try-catch block cannot catch Promise rejections because void suppresses the Promise—rejections will go unhandled.
Replace with proper async/await handling or explicit .catch() chaining. This pattern appears in multiple files (deepchatAgentPresenter/index.ts:2577 and newAgentPresenter/index.ts:1022).
Additionally, per coding guidelines (src/main/**/*.ts should "maintain centralized event handling in eventbus.ts"), consider emitting an event and letting the floating-button presenter react via the event bus instead of direct presenter coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/deepchatAgentPresenter/index.ts` around lines 2576 - 2580,
The current try/catch around
presenter.floatingButtonPresenter.refreshWidgetState() cannot catch rejections
because refreshWidgetState() returns a Promise and is being suppressed with
void; update the call to either await
presenter.floatingButtonPresenter.refreshWidgetState() inside an async function
or call presenter.floatingButtonPresenter.refreshWidgetState().catch(error =>
processLogger.warn(...)) so rejections are handled explicitly, and additionally
prefer emitting a centralized event (e.g., via
eventBus.emit('FloatingWidgetRefresh')) instead of directly coupling presenters
so the floating-button presenter listens and reacts to the event; apply the same
fix pattern where presenter's async calls are suppressed (notably
presenter.floatingButtonPresenter.refreshWidgetState and the analogous call in
newAgentPresenter).
Comment on lines
+484
to
+488
| const managedWindowPresenter = windowPresenter as typeof windowPresenter & { | ||
| windows: Map<number, BrowserWindow> | ||
| } | ||
|
|
||
| return managedWindowPresenter.windows.get(createdWindowId) ?? null |
Contributor
There was a problem hiding this comment.
Fragile access to internal windows Map via type assertion.
The code accesses windowPresenter.windows by casting to a type that includes a private Map. This is fragile and may break if the internal implementation changes.
💡 Suggestion
Consider adding a public method to windowPresenter like getWindowById(id: number): BrowserWindow | null instead of accessing the internal Map directly. This would provide a stable API contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/floatingButtonPresenter/index.ts` around lines 484 - 488,
Replace the fragile type-assertion access to the internal Map by adding a stable
public accessor on windowPresenter (e.g., add a method getWindowById(id:
number): BrowserWindow | null on the windowPresenter implementation) and call
that method instead of using (windowPresenter as
...).windows.get(createdWindowId); update the call site to use
windowPresenter.getWindowById(createdWindowId) and remove the type assertion/Map
access to avoid relying on private internals.
Comment on lines
+91
to
+107
| onSnapshotUpdate: (callback: (snapshot: FloatingWidgetSnapshot) => void) => { | ||
| ipcRenderer.on(FLOATING_BUTTON_EVENTS.SNAPSHOT_UPDATED, (_event, snapshot) => { | ||
| callback(snapshot) | ||
| }) | ||
| }, | ||
|
|
||
| onLanguageChanged: (callback: (language: string) => void) => { | ||
| ipcRenderer.on(FLOATING_BUTTON_EVENTS.LANGUAGE_CHANGED, (_event, language) => { | ||
| callback(language) | ||
| }) | ||
| }, | ||
|
|
||
| onThemeChanged: (callback: (theme: 'dark' | 'light') => void) => { | ||
| ipcRenderer.on(FLOATING_BUTTON_EVENTS.THEME_CHANGED, (_event, theme) => { | ||
| callback(theme) | ||
| }) | ||
| }, |
Contributor
There was a problem hiding this comment.
Event listeners can accumulate if called multiple times.
The onSnapshotUpdate, onLanguageChanged, and onThemeChanged methods add new listeners each time they're called without removing previous ones. If the renderer component is remounted, this could lead to duplicate callbacks.
🛡️ Proposed fix to prevent listener accumulation
onSnapshotUpdate: (callback: (snapshot: FloatingWidgetSnapshot) => void) => {
+ ipcRenderer.removeAllListeners(FLOATING_BUTTON_EVENTS.SNAPSHOT_UPDATED)
ipcRenderer.on(FLOATING_BUTTON_EVENTS.SNAPSHOT_UPDATED, (_event, snapshot) => {
callback(snapshot)
})
},
onLanguageChanged: (callback: (language: string) => void) => {
+ ipcRenderer.removeAllListeners(FLOATING_BUTTON_EVENTS.LANGUAGE_CHANGED)
ipcRenderer.on(FLOATING_BUTTON_EVENTS.LANGUAGE_CHANGED, (_event, language) => {
callback(language)
})
},
onThemeChanged: (callback: (theme: 'dark' | 'light') => void) => {
+ ipcRenderer.removeAllListeners(FLOATING_BUTTON_EVENTS.THEME_CHANGED)
ipcRenderer.on(FLOATING_BUTTON_EVENTS.THEME_CHANGED, (_event, theme) => {
callback(theme)
})
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/preload/floating-preload.ts` around lines 91 - 107, The three methods
onSnapshotUpdate, onLanguageChanged, and onThemeChanged add persistent
ipcRenderer listeners each call and can accumulate duplicates; modify each to
first remove any previous listener for the same FLOATING_BUTTON_EVENTS event (or
use ipcRenderer.once) and return an unsubscribe function so callers can clean up
when the renderer unmounts. Specifically update the implementations that
register FLOATING_BUTTON_EVENTS.SNAPSHOT_UPDATED, .LANGUAGE_CHANGED, and
.THEME_CHANGED to either (a) call ipcRenderer.removeListener for the same
callback before adding, or (b) use a wrapped one-time listener and provide a
cleanup function that calls ipcRenderer.removeListener to unregister the
handler.
Comment on lines
+229
to
+233
| <img | ||
| src="../src/assets/logo.png" | ||
| :alt="t('chat.floatingWidget.title')" | ||
| class="logo-orb-image status-orb-logo-image h-9 w-9" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the logo.png asset exists and how it's referenced elsewhere
fd -t f 'logo.png' --exec echo "Found: {}"
# Check how other components reference this asset
rg -n 'logo\.png' --type vue --type ts -C2Repository: ThinkInAIXYZ/deepchat
Length of output: 151
🏁 Script executed:
# Check the directory structure around the component and asset
tree -L 3 src/renderer/floating/ 2>/dev/null || find src/renderer/floating -type f | head -20
echo "---"
tree -L 3 src/renderer/src/assets/ 2>/dev/null || find src/renderer/src/assets -type f | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 10162
🏁 Script executed:
# Find other Vue files and check how they reference assets
find src/renderer -name "*.vue" -type f | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 556
🏁 Script executed:
# Look for img src patterns in Vue files
rg -n 'src=' --type-list | head -5
rg -n '<img' src/renderer -C 2Repository: ThinkInAIXYZ/deepchat
Length of output: 11599
🏁 Script executed:
# Check for vite or webpack config
find . -maxdepth 2 -name "vite.config.*" -o -name "webpack.config.*" -o -name "vitest.config.*" | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 111
Use the @/assets/ module alias instead of a relative path.
The path ../src/assets/logo.png resolves correctly to the existing asset file, but it's inconsistent with how other components reference assets throughout the codebase. All other Vue components (e.g., WelcomePage.vue, AboutUsSettings.vue, loading.vue) use the module alias @/assets/logo.png. Update lines 230 and 282 to use the same pattern for consistency and maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/floating/FloatingButton.vue` around lines 229 - 233, Replace the
relative asset import with the project alias in FloatingButton.vue: change the
img src attributes that currently use "../src/assets/logo.png" (the <img>
element with class "logo-orb-image status-orb-logo-image h-9 w-9" and the other
instance later in the file) to use "@/assets/logo.png" so they match the rest of
the codebase's asset imports.
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
2 participants
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
Behavior Changes
UI
Internationalization
Tests