fix: retry electric proxy and delay key toast #63
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR addresses two production issues: (1) automatic fallback from legacy Electric proxy port3010 to port 3000 when the configured host fails, and (2) preventing a premature "OpenRouter key missing" toast during initial page load. The server-side change extracts URL-building logic into a reusable helper and iterates over multiple base URL candidates, treating 5xx responses and connection errors as transient failures that trigger a retry. The client-side change introduces a checkedApiKey flag to guard the toast effect from firing before the stored key finishes loading. These changes fit into the existing architecture by enhancing the Electric shape proxy's resilience to deployment transitions and improving the UX around API key initialization in the chat room component.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| apps/server/src/index.ts | 3/5 | Added Electric proxy retry logic with fallback to port 3000; extracted URL builder into helper function |
| apps/web/src/components/chat-room.tsx | 1/5 | Introduced checkedApiKey flag to defer missing-key toast, but flag is set before async load completes causing timing bug |
Confidence score: 2/5
- This PR contains a critical timing bug in the client-side toast guard logic that prevents the intended fix from working correctly
- Score reflects one file with correct implementation (server retry) and one file with a logic error that breaks the feature (client toast). The server change is well-structured but has moderate complexity risk; the client change does not accomplish its stated goal due to the flag being set too early in the async flow
- Pay close attention to apps/web/src/components/chat-room.tsx lines 155-167 where
setCheckedApiKey(true)is called immediately after initiating the async load rather than after it completes
Issue 1: Toast guard timing bug (5/5 severity) - In apps/web/src/components/chat-room.tsx, line 162 sets checkedApiKey = true immediately after calling loadOpenRouterKey(), not after the promise resolves. Since the effect runs synchronously and the state update happens before the load completes, the guard on line 167 will pass on the second render even if apiKey is still null. The fix should move setCheckedApiKey(true) to line 161 (after the if (stored) block completes), ensuring it runs only after the async operation finishes. This bug breaks the entire intent of the PR's client-side change.
Issue 2: Retry logic treats 4xx as success (3/5 severity) - In apps/server/src/index.ts, line 407 breaks the retry loop for any response.status < 500, which includes client errors like 400, 401, 403, 404. While this may be intentional to avoid masking client issues, it means a misconfigured Electric endpoint returning 404 on the first candidate will prevent the fallback to port 3000. Consider whether only 2xx/3xx should break immediately, or add a comment explaining why 4xx are treated as non-retriable.
Issue 3: chatIdParam hoisting may be undefined (2/5 severity) - In apps/server/src/index.ts, line 349 hoists let chatIdParam: string | undefined outside the switch, but it is only assigned inside the "messages" case (line 390). If scope is "messages", the variable will be defined; however, the code relies on the reader understanding that buildTarget is only called when scope matches. Consider initializing to undefined explicitly or adding a runtime check in buildTarget to guard against undefined usage.
Issue 4: No test coverage for new retry logic (2/5 severity) - The PR description states "bun test (not run)" and the new server retry logic is untested. The fallback behavior is complex (multiple candidates, 5xx detection, error handling) and would benefit from unit tests verifying: (a) successful first-candidate response, (b) fallback to second candidate on 5xx, (c) 504 when all fail, (d) immediate break on 4xx. This is especially important for production-facing proxy code.
Sequence Diagram
sequenceDiagram
participant User
participant ChatRoom
participant Browser
participant Server
participant Electric
participant Database
User->>ChatRoom: "Load chat page"
ChatRoom->>Browser: "loadOpenRouterKey()"
Browser-->>ChatRoom: "stored API key or null"
alt API key exists
ChatRoom->>Server: "POST /api/openrouter/models"
Server-->>ChatRoom: "model list"
ChatRoom->>ChatRoom: "setCheckedApiKey(true)"
else No API key
ChatRoom->>ChatRoom: "setCheckedApiKey(true)"
ChatRoom->>User: "toast.warning('Add your OpenRouter API key')"
end
User->>ChatRoom: "Enter message and click send"
ChatRoom->>Server: "POST /api/chat"
Server->>Server: "Check rate limit"
alt Rate limited
Server-->>ChatRoom: "429 Too Many Requests"
else Not rate limited
Server->>Server: "createContext() - validate auth"
Server->>Database: "Upsert user message"
Server->>Server: "Stream AI response"
Server-->>ChatRoom: "Stream response chunks"
ChatRoom->>User: "Display streaming response"
Server->>Database: "Upsert assistant message"
end
User->>ChatRoom: "WebSocket connection established"
ChatRoom->>Server: "ws://server/sync?tabId=..."
Server->>Server: "createContext() - validate session"
alt Unauthorized
Server-->>ChatRoom: "close(1008, 'unauthorized')"
else Authorized
Server->>ChatRoom: "system.hello envelope"
ChatRoom->>Server: "['sub', {topic: 'chats:index:userId'}]"
Server->>Database: "Verify chat ownership"
Server->>Server: "hub.subscribe(ws, topic)"
Server-->>ChatRoom: "Subscription confirmed"
end
Server->>Electric: "GET /v1/shape (primary host)"
alt Primary host fails
Server->>Electric: "GET /v1/shape (fallback :3000)"
Electric-->>Server: "Shape data stream"
else Primary succeeds
Electric-->>Server: "Shape data stream"
end
Server-->>ChatRoom: "Proxied shape response"
ChatRoom->>User: "Update UI with synced data"
Context used:
- Context from
dashboard- AGENTS.md (source)
2 files reviewed, 3 comments
| }, [fetchModels]); | ||
|
|
||
| useEffect(() => { | ||
| if (!checkedApiKey) return; |
There was a problem hiding this comment.
logic: Early return prevents toast from ever showing – the checkedApiKey flag is set to true (line 162) before the conditional check, so this guard will never trigger on the initial mount when apiKey is null. Did you intend to set checkedApiKey after the API key check completes, or should the guard check happen after state updates?
| const baseCandidates = [ELECTRIC_BASE_URL]; | ||
| const baseWithoutPort = ELECTRIC_BASE_URL.replace(/:\\d+$/, ""); | ||
| const candidate3000 = `${baseWithoutPort}:3000`; | ||
| if (!baseCandidates.includes(candidate3000)) { | ||
| baseCandidates.push(candidate3000); | ||
| } |
There was a problem hiding this comment.
logic: The fallback logic adds :3000 to the base URL even if it already has a different port. If ELECTRIC_BASE_URL is http://example.com:3010, this creates http://example.com:3000. Is this the intended behavior, or should it skip the fallback when a port is explicitly configured? Should the fallback only apply when no explicit port is configured in ELECTRIC_BASE_URL?
| if (response.status < 500) { | ||
| upstreamResponse = response; | ||
| break; | ||
| } | ||
| lastError = new Error(`electric responded ${response.status}`); |
There was a problem hiding this comment.
logic: Treats any 5xx response as a failure and retries the next candidate. This means a legitimate503 from Electric will trigger a retry to :3000, which may produce a confusing error if :3000 is also unavailable or returns a different 5xx. Consider whether 5xx responses should be returned immediately or if only connection errors should trigger retry. Should server errors (5xx) from Electric trigger a retry, or only network/connection failures?
Summary
Testing