-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: notify renderer when api server ready #11049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes race condition issues with API server status synchronization by implementing an event-driven approach. Instead of relying on optimistic state updates, the renderer now waits for explicit "ready" events from the main process when the API server actually starts.
- Changed initial state in
useApiServerfrom optimistic to conservative (false instead of config-based) - Added IPC event mechanism to notify renderer when API server is ready
- Modified SWR key in
useAgentsto be null when server is not running, preventing premature fetch attempts
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/renderer/src/hooks/useApiServer.ts | Changed initial state to false and added effect to listen for API server ready events |
| src/renderer/src/hooks/agents/useAgents.ts | Made SWR key conditional on server running status to prevent fetching when server is down |
| src/preload/index.ts | Added onReady method to expose IPC event listener for API server ready notifications |
| src/main/apiServer/server.ts | Added notification to renderer when API server successfully starts listening |
| packages/shared/IpcChannel.ts | Added new IPC channel enum value for API server ready event |
Comments suppressed due to low confidence (2)
src/main/apiServer/server.ts:89
- The
restart()method callsstart()which will send theApiServer_Readyevent, but this may cause issues if the renderer isn't properly synchronized. The restart flow already callscheckApiServerStatus()in the renderer'srestartApiServercallback, which means the ready event listener will also triggercheckApiServerStatus(), resulting in two concurrent status checks. While not breaking, this creates unnecessary redundant API calls. Consider if the restart event needs special handling or if the ready event should be suppressed during restarts.
async restart(): Promise<void> {
await this.stop()
await config.reload()
await this.start()
}
src/renderer/src/hooks/agents/useAgents.ts:39
- The error handling logic in the fetcher is now redundant since
swrKeyis set tonullwhenapiServerRunningis false. SWR will not call the fetcher when the key is null, so lines 34-39 can never execute withapiServerRunningbeing false. Consider removing theapiServerRunningchecks from the fetcher to simplify the code, keeping only theapiServerConfig.enabledcheck if it serves a different purpose.
// Create a more dynamic SWR key that includes server running status
const swrKey = apiServerRunning ? key : null
const fetcher = useCallback(async () => {
// API server will start on startup if enabled OR there are agents
if (!apiServerConfig.enabled && !apiServerRunning) {
throw new Error(t('apiServer.messages.notEnabled'))
}
if (!apiServerRunning) {
throw new Error(t('agent.server.error.not_running'))
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
Note This issue/comment/review was translated by Claude. The UI needs some fine-tuning. Currently, it shows an Alert first because it detected 'not running', but it didn't check the loading state, which is actually the actual state. Original ContentUI要微调一下,现在先显示Alert,因为检查到 not running了,没有检查loading,但是实际上是loading的状态 |
What this PR does
Fixes the race condition where the agent list doesn't render if the API server starts slower than the page load, forcing users to manually switch tab to see the list.
Before this PR:
before.mov
After this PR:
after.mov
Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Breaking changes
If this PR introduces breaking changes, please describe the changes and the impact on users.
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note