Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a developer welcome override and new localized WelcomePage with settings-window navigation, emits SESSION_EVENTS.LIST_UPDATED when ACP agents change, makes session-building resilient to agent failures, removes legacy WelcomeView, and updates tests and i18n for the new welcome flow. Changes
Sequence Diagram(s)sequenceDiagram
participant WelcomePage
participant PageRouter
participant Router
participant WindowPresenter
participant SettingsWindow
participant Renderer
WelcomePage->>PageRouter: setSetting('init_complete', true) & goToNewThread()
PageRouter->>Router: router.replace({ name: 'chat', sessionId })
WelcomePage->>WindowPresenter: createSettingsWindow()
WindowPresenter->>SettingsWindow: create/focus window (settings-provider | settings-acp)
WelcomePage->>SettingsWindow: sendToWindow(SESSION_EVENTS.NAVIGATE, { route })
SettingsWindow->>Renderer: navigate to route (settings-provider | settings-acp)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/configPresenter/index.ts (1)
1305-1308:⚠️ Potential issue | 🟠 MajorSend
LIST_UPDATEDafter ACP refresh finishes.
SESSION_EVENTS.LIST_UPDATEDnow makes the renderer callfetchSessions()immediately, buthandleAcpAgentsMutated()still emits that event beforerefreshAcpProviderAgents(agentIds)completes. That means session reloads can race againstacpProvider.refreshAgents()and observe stale ACP availability, with no follow-up event to correct the UI.Suggested fix
- private handleAcpAgentsMutated(agentIds?: string[]) { + private async handleAcpAgentsMutated(agentIds?: string[]): Promise<void> { this.clearProviderModelStatusCache('acp') - this.notifyAcpAgentsChanged() - this.refreshAcpProviderAgents(agentIds) + await this.refreshAcpProviderAgents(agentIds) + this.notifyAcpAgentsChanged() } - private refreshAcpProviderAgents(agentIds?: string[]): void { + private async refreshAcpProviderAgents(agentIds?: string[]): Promise<void> { try { const providerInstance = presenter?.llmproviderPresenter?.getProviderInstance?.('acp') if (!providerInstance) { return } const acpProvider = providerInstance as AcpProvider if (typeof acpProvider.refreshAgents !== 'function') { return } - void acpProvider.refreshAgents(agentIds) + await acpProvider.refreshAgents(agentIds) } catch (error) { console.warn('[ACP] Failed to refresh agent processes after config change:', error) } }Also applies to: 1329-1332
🤖 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 1305 - 1308, The handler handleAcpAgentsMutated currently clears cache, emits notifyAcpAgentsChanged (which causes SESSION_EVENTS.LIST_UPDATED) and then calls refreshAcpProviderAgents, causing a race; change the flow so you clear the cache, call refreshAcpProviderAgents(agentIds) and await its completion, then call notifyAcpAgentsChanged (so SESSION_EVENTS.LIST_UPDATED is emitted only after refresh finishes). Apply the same change to the analogous block that follows (the other handler that calls notify... then refresh... around lines referenced in the review).src/main/presenter/newAgentPresenter/index.ts (1)
367-380:⚠️ Potential issue | 🟠 MajorDon’t collapse state-load failures into “session missing”.
tryBuildSessionWithState()now catches every failure path, sogetSession()returnsnullfor transientgetSessionState()errors as well as genuinely unavailable agents. That makesgetActiveSession()clear a valid binding on a temporary runtime hiccup, and callers likesrc/main/presenter/mcpPresenter/toolManager.ts:426-432will silently skip ACP agent propagation because they only seenull. Please keep the skip-only behavior for permanently unavailable agents in list APIs, but let single-session lookups surface non-agent-resolution failures instead.Suggested direction
async getSession(sessionId: string): Promise<SessionWithState | null> { const record = this.sessionManager.get(sessionId) if (!record) return null - return await this.tryBuildSessionWithState(record) + return await this.buildSessionWithState(record) } private async tryBuildSessionWithState(record: SessionRecord): Promise<SessionWithState | null> { try { return await this.buildSessionWithState(record) } catch (error) { + if (!(error instanceof Error) || !error.message.startsWith('Agent not found:')) { + throw error + } console.warn( `[NewAgentPresenter] Skipping unavailable session id=${record.id} agent=${record.agentId}:`, error ) return null } }Also applies to: 545-552, 898-907
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/newAgentPresenter/index.ts` around lines 367 - 380, getSession currently calls tryBuildSessionWithState(record) which swallows all errors, causing transient state-load failures to be treated the same as missing sessions; change getSession to call tryBuildSessionWithState(record) but propagate non-agent-resolution errors instead of returning null: specifically, keep the early null return for genuinely missing records from sessionManager.get(sessionId), but when tryBuildSessionWithState throws or returns an error indicating a transient getSessionState()/agent-resolution failure, rethrow that error (or return it to the caller) so callers (e.g., getActiveSession and the toolManager consumer) can detect and handle transient failures; update analogous call sites that rely on getSession (the other occurrences around tryBuildSessionWithState) to preserve list APIs behavior (filter out permanently unavailable agents) while surfacing non-resolution errors for single-session lookups.
🧹 Nitpick comments (7)
test/renderer/stores/sessionStore.test.ts (1)
221-229: Consider usingflushPromisesfor more reliable async handling.The
setTimeout(resolve, 0)pattern works but can be fragile if the async chain has multiple ticks. Consider using@vue/test-utils'sflushPromisesfor consistency with other Vue tests.♻️ Suggested improvement
+import { flushPromises } from '@vue/test-utils' + it('reloads sessions when the session list update event fires', async () => { const { newAgentPresenter, emitIpc, SESSION_EVENTS } = await setupStore() emitIpc(SESSION_EVENTS.LIST_UPDATED) - await new Promise((resolve) => setTimeout(resolve, 0)) + await flushPromises() expect(newAgentPresenter.getSessionList).toHaveBeenCalledTimes(1) expect(newAgentPresenter.getActiveSession).toHaveBeenCalledTimes(1) })🤖 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 221 - 229, Replace the ad-hoc tick wait with Vue's flushPromises to make the async test robust: import flushPromises from '@vue/test-utils' at the top, and in the test that uses setupStore()/emitIpc(SESSION_EVENTS.LIST_UPDATED) replace the await new Promise(resolve => setTimeout(resolve, 0)) with await flushPromises(); keep assertions that newAgentPresenter.getSessionList and getActiveSession were called once so the test still verifies those calls.src/renderer/src/App.vue (2)
231-231: Potential silent error suppression withvoidoperator.If
configPresenter.getSetting()or router operations fail, the error will be silently swallowed. Consider adding error handling to log failures while still allowing the UI to proceed.♻️ Suggested improvement
-void ensureStartupWelcomeState() +ensureStartupWelcomeState().catch((error) => { + console.error('Startup welcome state check failed:', error) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/App.vue` at line 231, The call to ensureStartupWelcomeState uses the void operator which can silently swallow rejections from async operations like configPresenter.getSetting() and router navigation; update ensureStartupWelcomeState (or the place that calls it) to await its result inside a try/catch (or attach a .catch) and log any errors via the existing logger, ensuring failures from configPresenter.getSetting() and router.push/replace are surfaced while allowing the UI flow to continue.
29-29: Consider extracting the constant to a shared location.
DEV_WELCOME_OVERRIDE_KEYis duplicated betweensrc/preload/index.tsandsrc/renderer/src/App.vue. While sharing between preload and renderer contexts can be complex, a shared constants file undersrc/shared/would ensure consistency and prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/App.vue` at line 29, DEV_WELCOME_OVERRIDE_KEY is duplicated; extract it into a shared constants module (e.g., export const DEV_WELCOME_OVERRIDE_KEY = '__deepchat_dev_force_welcome') and update both locations to import that symbol instead of redefining it. Create the new module under a shared directory (e.g., src/shared/) and ensure both preload code (where DEV_WELCOME_OVERRIDE_KEY is used) and the renderer App.vue import from that module (adjust build/import aliases if needed so preload and renderer can resolve the shared file).test/renderer/components/App.startup.test.ts (1)
197-207: Note: Test relies on implicitimport.meta.env.DEVbeing true in Vitest.The dev override test works because Vitest sets
import.meta.env.DEV = trueby default. Consider adding a comment or explicit mock to make this dependency clear, preventing silent failures if test configuration changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/App.startup.test.ts` around lines 197 - 207, The test relies on import.meta.env.DEV being true (Vitest default), so explicitly document or mock it to avoid silent failures: either add a one-line comment above the test noting the dependency on import.meta.env.DEV for future maintainers, or explicitly set the env for this test (e.g., use Vitest's vi.stubEnv/fixture to ensure DEV=true) before calling mountApp; reference DEV_WELCOME_OVERRIDE_KEY and the mountApp invocation so the change is applied to this specific test case.src/renderer/src/pages/WelcomePage.vue (1)
111-120: Race condition workaround with 250ms retry is fragile and may not be sufficient.The settings window's Vue app mounts asynchronously after
createSettingsWindow()completes. The immediatenavigateToSettings()call may arrive before theSETTINGS_EVENTS.NAVIGATElistener is registered in the settings app (seesrc/renderer/settings/App.vue). Although the listener handler includesawait router.isReady()for synchronization, this only protects navigation routing—the listener registration itself is asynchronous. The 250ms retry is an arbitrary delay that may not be sufficient on slow machines or systems under load.Consider having the settings window emit a "ready" IPC event once fully initialized, or implementing a retry with confirmation that the message was received.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/pages/WelcomePage.vue` around lines 111 - 120, The retry with a fixed 250ms delay is fragile; modify the flow so navigation waits for an explicit "settings-ready" acknowledgement from the settings window instead of sleeping: update createSettingsWindow / windowPresenter to either return a promise that resolves when the settings window emits a ready IPC event (emitted by the settings app after it registers the SETTINGS_EVENTS.NAVIGATE listener) or add logic here to listen for that IPC "settings-ready" message (using ipcRenderer.once) before calling navigateToSettings(settingsWindowId, routeName); remove the setTimeout retry and ensure the settings app emits the ready event after registering its SETTINGS_EVENTS.NAVIGATE handler (see settings App.vue) so navigateToSettings is only called after the listener exists.src/renderer/src/components/mock/MockWelcomePage.vue (1)
92-103: Prefer thecreateSettingsWindow()result over a second presenter lookup.
openSettings()already waits for window creation, then immediately depends ongetSettingsWindowId()being up to date. If that cached id lags the promise resolution, both navigation sends are skipped even though the window exists.Possible simplification
const openSettings = async (routeName: SettingsRouteName) => { const windowId = window.api.getWindowId() if (windowId != null) { - await windowPresenter.createSettingsWindow() - const settingsWindowId = windowPresenter.getSettingsWindowId() + const settingsWindowId = + (await windowPresenter.createSettingsWindow()) ?? + windowPresenter.getSettingsWindowId() if (settingsWindowId != null) { navigateToSettings(settingsWindowId, routeName) window.setTimeout(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/mock/MockWelcomePage.vue` around lines 92 - 103, The openSettings function should use the created window id returned or captured immediately after awaiting windowPresenter.createSettingsWindow() rather than re-querying windowPresenter.getSettingsWindowId() later; update openSettings to store the settingsWindowId right after the await (or change createSettingsWindow() to return the new id) and use that stored settingsWindowId for both the immediate navigateToSettings(settingsWindowId, routeName) call and the setTimeout comparison, so you avoid a stale second lookup via getSettingsWindowId().test/renderer/components/WelcomePage.test.ts (1)
11-85: Extract the repeated WelcomePage bootstrap into a helper.The three cases repeat the same mocks and mount scaffolding almost verbatim, so any presenter or router contract change has to be updated in three places. A small
mountWelcomePage(...)helper would keep these tests easier to evolve.Also applies to: 106-180, 201-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/WelcomePage.test.ts` around lines 11 - 85, Extract the repeated test setup into a helper function (e.g., mountWelcomePage) that centralizes the mock wiring and mounting of WelcomePage: move the vi.resetModules/vi.useFakeTimers, the window.api stub, the vi.doMock calls for usePresenter (returning configPresenter/windowPresenter), usePageRouterStore, vue-router mocks (useRoute/useRouter), vue-i18n, icons and theme, and the mount(WelcomePage, ...) call into this helper; have mountWelcomePage return the mounted wrapper plus the mocked collaborators (router, pageRouter, configPresenter, windowPresenter) so each test can call mountWelcomePage() and assert behavior (e.g., calls to router.replace, pageRouter.goToNewThread, configPresenter.setSetting, windowPresenter.createSettingsWindow) without duplicating the scaffolding.
🤖 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/renderer/src/stores/ui/pageRouter.ts`:
- Around line 16-31: The initialize() function leaves error.value set after a
failure so future successful retries still show the stale error; clear
error.value at the start of initialize() (or immediately on successful path) so
any prior error is removed before attempting to re-run. Update the initialize()
function to reset error.value = '' (or null) before calling
newAgentPresenter.getActiveSession(...) and/or set error.value = '' right after
you set route.value on success (e.g., when setting route to { name: 'chat',
sessionId: ... } or { name: 'newThread' }) to ensure the UI no longer displays
the old failure message.
---
Outside diff comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1305-1308: The handler handleAcpAgentsMutated currently clears
cache, emits notifyAcpAgentsChanged (which causes SESSION_EVENTS.LIST_UPDATED)
and then calls refreshAcpProviderAgents, causing a race; change the flow so you
clear the cache, call refreshAcpProviderAgents(agentIds) and await its
completion, then call notifyAcpAgentsChanged (so SESSION_EVENTS.LIST_UPDATED is
emitted only after refresh finishes). Apply the same change to the analogous
block that follows (the other handler that calls notify... then refresh...
around lines referenced in the review).
In `@src/main/presenter/newAgentPresenter/index.ts`:
- Around line 367-380: getSession currently calls
tryBuildSessionWithState(record) which swallows all errors, causing transient
state-load failures to be treated the same as missing sessions; change
getSession to call tryBuildSessionWithState(record) but propagate
non-agent-resolution errors instead of returning null: specifically, keep the
early null return for genuinely missing records from
sessionManager.get(sessionId), but when tryBuildSessionWithState throws or
returns an error indicating a transient getSessionState()/agent-resolution
failure, rethrow that error (or return it to the caller) so callers (e.g.,
getActiveSession and the toolManager consumer) can detect and handle transient
failures; update analogous call sites that rely on getSession (the other
occurrences around tryBuildSessionWithState) to preserve list APIs behavior
(filter out permanently unavailable agents) while surfacing non-resolution
errors for single-session lookups.
---
Nitpick comments:
In `@src/renderer/src/App.vue`:
- Line 231: The call to ensureStartupWelcomeState uses the void operator which
can silently swallow rejections from async operations like
configPresenter.getSetting() and router navigation; update
ensureStartupWelcomeState (or the place that calls it) to await its result
inside a try/catch (or attach a .catch) and log any errors via the existing
logger, ensuring failures from configPresenter.getSetting() and
router.push/replace are surfaced while allowing the UI flow to continue.
- Line 29: DEV_WELCOME_OVERRIDE_KEY is duplicated; extract it into a shared
constants module (e.g., export const DEV_WELCOME_OVERRIDE_KEY =
'__deepchat_dev_force_welcome') and update both locations to import that symbol
instead of redefining it. Create the new module under a shared directory (e.g.,
src/shared/) and ensure both preload code (where DEV_WELCOME_OVERRIDE_KEY is
used) and the renderer App.vue import from that module (adjust build/import
aliases if needed so preload and renderer can resolve the shared file).
In `@src/renderer/src/components/mock/MockWelcomePage.vue`:
- Around line 92-103: The openSettings function should use the created window id
returned or captured immediately after awaiting
windowPresenter.createSettingsWindow() rather than re-querying
windowPresenter.getSettingsWindowId() later; update openSettings to store the
settingsWindowId right after the await (or change createSettingsWindow() to
return the new id) and use that stored settingsWindowId for both the immediate
navigateToSettings(settingsWindowId, routeName) call and the setTimeout
comparison, so you avoid a stale second lookup via getSettingsWindowId().
In `@src/renderer/src/pages/WelcomePage.vue`:
- Around line 111-120: The retry with a fixed 250ms delay is fragile; modify the
flow so navigation waits for an explicit "settings-ready" acknowledgement from
the settings window instead of sleeping: update createSettingsWindow /
windowPresenter to either return a promise that resolves when the settings
window emits a ready IPC event (emitted by the settings app after it registers
the SETTINGS_EVENTS.NAVIGATE listener) or add logic here to listen for that IPC
"settings-ready" message (using ipcRenderer.once) before calling
navigateToSettings(settingsWindowId, routeName); remove the setTimeout retry and
ensure the settings app emits the ready event after registering its
SETTINGS_EVENTS.NAVIGATE handler (see settings App.vue) so navigateToSettings is
only called after the listener exists.
In `@test/renderer/components/App.startup.test.ts`:
- Around line 197-207: The test relies on import.meta.env.DEV being true (Vitest
default), so explicitly document or mock it to avoid silent failures: either add
a one-line comment above the test noting the dependency on import.meta.env.DEV
for future maintainers, or explicitly set the env for this test (e.g., use
Vitest's vi.stubEnv/fixture to ensure DEV=true) before calling mountApp;
reference DEV_WELCOME_OVERRIDE_KEY and the mountApp invocation so the change is
applied to this specific test case.
In `@test/renderer/components/WelcomePage.test.ts`:
- Around line 11-85: Extract the repeated test setup into a helper function
(e.g., mountWelcomePage) that centralizes the mock wiring and mounting of
WelcomePage: move the vi.resetModules/vi.useFakeTimers, the window.api stub, the
vi.doMock calls for usePresenter (returning configPresenter/windowPresenter),
usePageRouterStore, vue-router mocks (useRoute/useRouter), vue-i18n, icons and
theme, and the mount(WelcomePage, ...) call into this helper; have
mountWelcomePage return the mounted wrapper plus the mocked collaborators
(router, pageRouter, configPresenter, windowPresenter) so each test can call
mountWelcomePage() and assert behavior (e.g., calls to router.replace,
pageRouter.goToNewThread, configPresenter.setSetting,
windowPresenter.createSettingsWindow) without duplicating the scaffolding.
In `@test/renderer/stores/sessionStore.test.ts`:
- Around line 221-229: Replace the ad-hoc tick wait with Vue's flushPromises to
make the async test robust: import flushPromises from '@vue/test-utils' at the
top, and in the test that uses setupStore()/emitIpc(SESSION_EVENTS.LIST_UPDATED)
replace the await new Promise(resolve => setTimeout(resolve, 0)) with await
flushPromises(); keep assertions that newAgentPresenter.getSessionList and
getActiveSession were called once so the test still verifies those calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46a0dfda-2a0a-4c7c-aafb-2aedae9aae89
📒 Files selected for processing (29)
src/main/presenter/configPresenter/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/preload/index.d.tssrc/preload/index.tssrc/renderer/src/App.vuesrc/renderer/src/components/mock/MockWelcomePage.vuesrc/renderer/src/i18n/da-DK/welcome.jsonsrc/renderer/src/i18n/en-US/welcome.jsonsrc/renderer/src/i18n/fa-IR/welcome.jsonsrc/renderer/src/i18n/fr-FR/welcome.jsonsrc/renderer/src/i18n/he-IL/welcome.jsonsrc/renderer/src/i18n/ja-JP/welcome.jsonsrc/renderer/src/i18n/ko-KR/welcome.jsonsrc/renderer/src/i18n/pt-BR/welcome.jsonsrc/renderer/src/i18n/ru-RU/welcome.jsonsrc/renderer/src/i18n/zh-CN/welcome.jsonsrc/renderer/src/i18n/zh-HK/welcome.jsonsrc/renderer/src/i18n/zh-TW/welcome.jsonsrc/renderer/src/pages/WelcomePage.vuesrc/renderer/src/router/index.tssrc/renderer/src/stores/ui/pageRouter.tssrc/renderer/src/stores/ui/session.tssrc/renderer/src/views/ChatTabView.vuesrc/renderer/src/views/WelcomeView.vuetest/main/presenter/newAgentPresenter/newAgentPresenter.test.tstest/renderer/components/App.startup.test.tstest/renderer/components/WelcomePage.test.tstest/renderer/stores/pageRouter.test.tstest/renderer/stores/sessionStore.test.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/views/WelcomeView.vue
This was referenced Mar 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#157
Summary by CodeRabbit
New Features
Bug Fixes
Refactor