feat: add Codex local provider integration for desktop#307
Open
feat: add Codex local provider integration for desktop#307
Conversation
Adds Codex (OpenAI Codex CLI) as a local provider option in the desktop app, communicating via Tauri IPC + stdio JSON-RPC. Includes fixes for: - Race condition in app-server spawn (compare_exchange instead of load/store) - Orphaned process on app exit (RunEvent::Exit cleanup with SIGTERM) - Stdin handle not cleared on crash (codex_mark_stopped resets all state) - Explicit stderr piping so logs are always captured - Message flicker on stream finish (suppress Convex sync echo-back for 2s) - Update selectModel tests to match paid-user model override in agent mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds client-side Codex local-provider support: new CodexLocalTransport and DelegatingTransport, Tauri IPC and sidecar lifecycle, model-selector UI/locking and Codex availability checks, Convex schema/mutations for persisting local chats/messages, Codex tool handling and sidebar parsing, plus related hooks, types, and desktop changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Web Client
participant Delegator as DelegatingTransport
participant Codex as CodexLocalTransport
participant AppServer as Codex App-Server
participant Convex
User->>Client: Select model & send message
Client->>Client: isCodexLocal(selectedModel)?
alt Codex local selected
Client->>Delegator: sendMessages()
Delegator->>Codex: sendMessages()
Codex->>AppServer: start thread / send via Tauri RPC
AppServer-->>Codex: stream events (text, tool notifications, diffs)
Codex-->>Client: stream UIMessageChunks
Client->>Convex: persistCodexMessages() / saveLocalChat()
Convex-->>Client: ack
else Remote provider
Client->>Delegator: sendMessages()
Delegator->>ClientTransport: delegate to DefaultChatTransport
ClientTransport->>Server: server-side processing
Server-->>ClientTransport: response
ClientTransport-->>Client: render response
end
Client->>User: render response + tools/sidebar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
app/components/DiffView.tsx (1)
23-39: Type the editor ref and mount callback instead of usingany.Line 23 and Line 37 currently use
anyand lose type safety with theDiffEditorcomponent. TheDiffEditor'sonMountcallback from@monaco-editor/reactv4.7.0 is typed as(editor: IStandaloneDiffEditor, monaco: Monaco) => void, allowing you to infer precise types directly from the component props.♻️ Proposed refactor
- const editorRef = useRef<any>(null); + type DiffEditorOnMount = NonNullable< + React.ComponentProps<typeof DiffEditor>["onMount"] + >; + const editorRef = useRef<Parameters<DiffEditorOnMount>[0] | null>(null); @@ - const handleEditorMount = (editor: any) => { + const handleEditorMount: DiffEditorOnMount = (editor) => { editorRef.current = editor; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DiffView.tsx` around lines 23 - 39, The editorRef and handleEditorMount are typed as any losing type safety; change editorRef to use React.RefObject<IStandaloneDiffEditor | null> and type the mount callback handleEditorMount as (editor: IStandaloneDiffEditor, monaco: Monaco) => void (or infer from DiffEditorProps) so the ref holds IStandaloneDiffEditor methods and dispose is correctly typed; update imports to bring in IStandaloneDiffEditor and Monaco from 'monaco-editor' and replace any occurrences of editorRef and handleEditorMount to use the new typed signatures.app/contexts/GlobalState.tsx (1)
261-266: Unused dependency inuseCallback.
chatModeis listed in the dependency array but is not referenced in the callback body. This is harmless but could be cleaned up.♻️ Suggested fix
const setSelectedModelState = useCallback( (model: SelectedModel) => { setSelectedModelRaw(model); }, - [chatMode], + [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/contexts/GlobalState.tsx` around lines 261 - 266, The useCallback for setSelectedModelState lists chatMode in its dependency array but the callback only calls setSelectedModelRaw; remove chatMode and include the actual dependency (setSelectedModelRaw) instead. Update the dependency array for setSelectedModelState to [setSelectedModelRaw] (or [] only if setSelectedModelRaw is guaranteed stable) so React hooks linting is satisfied and the callback dependencies are correct.lib/chat/__tests__/chat-processor.test.ts (1)
184-209: LGTM!Test cases correctly updated to reflect the new agent-mode model override behavior for paid users. Good coverage of edge cases including "auto" and undefined overrides.
Consider adding a test case to verify that
selectModelthrows when passed a"codex-local"or"codex-local:*"model identifier. This would ensure the server-side guard inselectModelis tested.Would you like me to generate the missing test case for the
codex-localrejection guard?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/chat/__tests__/chat-processor.test.ts` around lines 184 - 209, Add a unit test for selectModel that asserts it throws when given disallowed local identifiers: call selectModel("agent" or "agent-long", any paid tier like "pro", with "codex-local" and with a namespaced variant like "codex-local:1") and expect an error to be thrown; reference the selectModel function in the new test so it explicitly verifies the server-side guard rejects "codex-local" and "codex-local:*" model identifiers.app/hooks/useChatHandlers.ts (1)
247-259: Consider error handling strategy for pre-save failure.The pre-save mutation catches and logs errors but continues with the stream. If
saveLocalChatMutationfails (e.g., Convex is unreachable), the chat will stream but won't be persisted, potentially losing the conversation.This fire-and-forget pattern is acceptable if the local provider handles its own persistence, but if Convex is the source of truth for local chat history, the user might lose their conversation without knowing. Consider showing a toast warning if the pre-save fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useChatHandlers.ts` around lines 247 - 259, Pre-save failures are currently swallowed in useChatHandlers.ts (inside the isCodexLocal branch) so streaming continues while the chat might not be persisted; update the catch block around saveLocalChatMutation to surface a user-visible warning (e.g., call your toast/notification helper like showToast or toast.error) that includes a short, friendly message plus the error detail, and keep the current behavior of continuing the stream; optionally add a lightweight retry/enqueue-for-later flag in the same block so the chat can be re-saved later if necessary.app/components/ModelSelector/checkCodexStatus.ts (2)
5-9: Cache invalidation may cause stale authentication state.The cache stores
{ installed: true, authenticated, version }on first successful check and never invalidates. If a user:
- Opens the app with Codex installed and authenticated → cached as authenticated
- Logs out of Codex CLI externally
- Tries to use Codex in the app → cached value still shows authenticated
Consider either:
- Not caching the
authenticatedstate (only cacheinstalledandversion)- Adding a manual cache invalidation function for retry scenarios
- Adding a TTL to the cache
🔧 Suggested modification to avoid caching authentication state
let _cache: { installed: boolean; - authenticated: boolean; version?: string; } | null = null; export async function checkCodexStatus(): Promise<{ installed: boolean; authenticated: boolean; version?: string; } | null> { - if (_cache) { - return _cache; + // Only cache installation status, always re-check authentication + if (_cache) { + // Re-check auth status even when cached + const { invoke } = await import("@tauri-apps/api/core"); + const authResult = await invoke<{ + stdout: string; + stderr: string; + exit_code: number; + }>("execute_command", { + command: "codex login status", + timeoutMs: 5000, + }); + return { ..._cache, authenticated: authResult.exit_code === 0 }; } // ... rest of function - _cache = { installed: true, authenticated, version }; + _cache = { installed: true, version }; - return _cache; + return { ..._cache, authenticated }; }Also applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ModelSelector/checkCodexStatus.ts` around lines 5 - 9, The current module-level _cache object (named _cache) stores the authenticated flag permanently causing stale state; change checkCodexStatus to stop persisting authenticated in _cache (only cache installed and version) and compute authenticated on each call, or alternatively add an explicit invalidate function (e.g., export function invalidateCodexCache()) or implement a TTL timestamp on _cache so authenticated is rechecked after expiry; update references to _cache and the checkCodexStatus function to use the new shape/invalidator and ensure any callers can trigger invalidateCodexCache() when a retry or logout flow occurs.
39-42: Non-zero exit code does not necessarily mean "not found".
codex --versionreturning a non-zero exit code could indicate the CLI crashed, has a bug, or failed for reasons other than not being installed. The current logic logs "Codex CLI not found" which may be misleading.Consider checking stderr for common "command not found" patterns or adjusting the log message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ModelSelector/checkCodexStatus.ts` around lines 39 - 42, The code in checkCodexStatus.ts currently treats any non-zero versionResult.exit_code as "Codex CLI not found"; update the logic in the checkCodexStatus function to distinguish between a missing executable and other failures by inspecting versionResult.stderr (and any error code if present) for common "not found" patterns (e.g., "command not found", "not recognized", "No such file or directory") or an ENOENT-like error, and only log/return installed: false when those patterns match; for other non-zero exits, change the log to indicate the CLI returned an error/crashed and return installed: true with authenticated: false (or propagate the error) so the message is not misleading.app/components/tools/CodexToolHandler.tsx (1)
19-22: Consider adding type safety forpartprop.Using
anyloses type safety. If the Codex tool part structure is known, consider defining a more specific type or union type for better IDE support and compile-time checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/tools/CodexToolHandler.tsx` around lines 19 - 22, The CodexToolHandlerProps currently types part as any, losing type safety; define a specific interface or union (e.g., CodexToolPart or CodexToolPartUnion) that lists the expected fields used by the component (for example id, type, content, metadata, status, etc.), replace the any with that new type in CodexToolHandlerProps, update the CodexToolHandler component signature and any internal usages to match the new fields, and export/import the type where needed so IDE and TypeScript checks verify correct usage across the codebase.lib/local-providers/codex-transport.ts (1)
620-625:rpcNotifypropagates invoke errors to caller.Unlike
rpcRequestwhich catches invoke errors,rpcNotifyawaits without try/catch. If the Tauri invoke fails, the error propagates to callers (e.g.,rpcNotify("initialized", {})on line 186). This may be intentional for fire-and-forget notifications, but consider whether silent failure is more appropriate for notifications.🛡️ Optional: swallow notification errors
private async rpcNotify(method: string, params: any): Promise<void> { const msg = JSON.stringify({ jsonrpc: "2.0", method, params }); console.log("[CodexTransport] → (notify)", method); - const { invoke } = await import("@tauri-apps/api/core"); - await invoke("codex_rpc_send", { message: msg }); + try { + const { invoke } = await import("@tauri-apps/api/core"); + await invoke("codex_rpc_send", { message: msg }); + } catch (err) { + console.warn("[CodexTransport] Notification failed:", method, err); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/local-providers/codex-transport.ts` around lines 620 - 625, Wrap the body of rpcNotify in a try/catch so Tauri invoke failures do not propagate to callers: inside the rpcNotify method (which builds msg and calls invoke("codex_rpc_send", { message: msg })), catch any error from the await invoke call and handle it silently or log it (e.g., console.error or the existing logger) instead of rethrowing; this preserves fire-and-forget semantics while keeping the descriptive console.log("[CodexTransport] → (notify)", method) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/chat.tsx`:
- Around line 689-699: The short-circuit that compares only message IDs (using
messagesRef.current and uiMessages) drops legitimate updates where a message's
content changed but its id/position did not; update the guard in the chat update
path (where messagesRef.current and uiMessages are compared before returning to
useChat) to either remove the ID-only fast path or broaden the comparison to
detect content changes — e.g., compare a stable message version field
(updatedAt/updatedTimestamp) or perform a shallow/deep equality check of message
objects (not just ids) so updates from api.messages.getMessagesByChatId
propagate to useChat.
- Around line 238-248: The callback must abort the send when the Codex sidecar
fails to start: after calling await ensureSidecarRef.current(), capture its
boolean return value and if it is false, stop further processing (do not call
codexTransport.sendMessages or proceed with the send). Update the async callback
that uses isCodexLocal(selectedModelRef.current) / getCodexSubModel(...) /
codexTransport.setModel(...) / codexTransport.setUserData(...) to check the
result of ensureSidecarRef.current() and return early on failure so the
transport is not invoked.
In `@app/components/DiffView.tsx`:
- Line 115: The DiffEditor is being forced to remount on every streaming update
because the key uses `${originalContent.length}-${modifiedContent.length}`;
remove that unstable key or replace it with a stable identifier from the
streaming payload (for example a revision or message id on resolvedFile or
toolExecutions) so the editor is not recreated on incremental content
changes—locate the DiffEditor render where the key prop is set
(`key={`${originalContent.length}-${modifiedContent.length}`}`) and either drop
the key entirely or set it to a stable property like `resolvedFile.revisionId`
or the message id that only changes for true revisions.
In `@app/components/ModelSelector.tsx`:
- Around line 119-125: The model selector buttons in ModelSelector.tsx are
currently bare <button> elements (e.g., the button that calls onSelect(option)
and the Codex submenu trigger) and thus default to type="submit" inside the chat
form; change each such button to explicitly include type="button" so clicks
don't submit the form—update the button in the group that renders
onSelect(option) and any other buttons in the component (the Codex submenu
trigger and the buttons rendered in the blocks around the referenced areas) to
include type="button".
In `@app/components/tools/CodexToolHandler.tsx`:
- Around line 24-33: The arePropsEqual function on CodexToolHandlerProps
currently omits part.input and part.type, causing stale renders; update
arePropsEqual (used for memoization of CodexToolHandlerProps) to also compare
prev.part.input !== next.part.input and prev.part.type !== next.part.type so
changes to input or type (which drive itemType and the sidebarContent useMemo)
trigger re-renders; keep the same short-circuit style as the existing checks.
- Around line 101-106: The returned runningAction/doneAction for the
"fileChange" case uses naive string concatenation (e.g., `${input?.action ||
"edit"}ing`) which produces incorrect forms; update this case to use a small
action-to-gerund and action-to-past mapping (or a helper like getGerund(action)
and getPastTense(action)) that covers common verbs such as "write" ->
"writing"/"wrote" or "delete" -> "deleting"/"deleted" and falls back to safe
defaults ("editing"/"edited") using input?.action to build runningAction,
doneAction, and keep target as before (input?.path || input?.file || "file");
modify the "fileChange" branch to call these helpers instead of appending "ing"
or "ed".
In `@app/hooks/useCodexLocal.ts`:
- Around line 27-70: The current early-return in ensureSidecar
(startedRef.current && ready) lets the hook skip re-checking the sidecar status
after a crash and allows races on transport.startListening; change ensureSidecar
to always verify the actual backend liveness by calling
get_codex_app_server_info (invoke) instead of trusting ready alone, and only
short-circuit when that probe confirms the server is running. Also serialize
concurrent starts by using the existing setStarting / starting flag or a small
mutex around the start path so multiple callers await the same start sequence
(protect transport.startListening and the invoke("start_codex_app_server")
sequence). Update references in ensureSidecar, startedRef, ready,
transport.startListening and the get_codex_app_server_info invoke call
accordingly.
In `@app/hooks/useCodexPersistence.ts`:
- Around line 71-76: The function in useCodexPersistence.ts that runs
Promise.all (lines ~39-54) currently logs errors in its catch block but still
returns true, incorrectly signaling success; change the catch block in that
function to either return false after logging the error or rethrow the error so
callers receive failure (i.e., replace the current console.error-only catch with
one that returns false or throws), and ensure the function’s callers handle the
boolean/error accordingly.
- Around line 59-63: Replace the silent swallow of errors on saveLocalChat with
error logging: in the block that calls saveLocalChat({ id: chatId, title: "",
codexThreadId: threadId }).catch(() => {}), change the catch to accept the error
and log it (include chatId and threadId for context) using the existing logger
or console.error so failures to persist the thread ID are visible; ensure the
call still does not throw but surfaces the error for debugging.
In `@convex/chats.ts`:
- Around line 205-223: When handling the upsert branch for existing chats in
convex/chats.ts, the patch only updates update_time and codex_thread_id but
never persists a changed selectedModel; update the patch construction in the
existing branch to add selected_model when args.selectedModel is provided (i.e.,
extend the patch Record to include selected_model: args.selectedModel) before
calling ctx.db.patch(existing._id, patch) so existing chats get their
selected_model updated.
In `@convex/messages.ts`:
- Around line 208-221: The deduplication query checks a global message id before
authorization, leaking whether an id exists; move the authorization check
(internal.messages.verifyChatOwnership with { chatId: args.chatId, userId:
user.subject }) to run before the ctx.db query that checks args.id, or
alternatively restrict the dedupe to the target chat by changing the lookup to
include the chat scope (e.g., use/replace the global withIndex("by_message_id")
call with a query that also filters/equates on args.chatId or use an index that
keys by (chatId, id)), so unauthorized callers cannot differentiate existence.
In `@lib/local-providers/codex-transport.ts`:
- Around line 339-341: The code only records the most recent fileChange tool via
lastFileChangeToolCallId, so when the turn/diff/updated event fires only that
single tool call gets the aggregated diff; change the tracking to collect all
fileChange IDs (e.g., replace lastFileChangeToolCallId with an array/Set like
fileChangeToolCallIds when detecting item.type === "fileChange"), add each
item.id to that collection, and then in the turn/diff/updated handler iterate
over fileChangeToolCallIds to apply the updated diff to every corresponding tool
call (and clear the collection after applying); update any references to
lastFileChangeToolCallId accordingly so earlier fileChange tool calls receive
their diffs too.
---
Nitpick comments:
In `@app/components/DiffView.tsx`:
- Around line 23-39: The editorRef and handleEditorMount are typed as any losing
type safety; change editorRef to use React.RefObject<IStandaloneDiffEditor |
null> and type the mount callback handleEditorMount as (editor:
IStandaloneDiffEditor, monaco: Monaco) => void (or infer from DiffEditorProps)
so the ref holds IStandaloneDiffEditor methods and dispose is correctly typed;
update imports to bring in IStandaloneDiffEditor and Monaco from 'monaco-editor'
and replace any occurrences of editorRef and handleEditorMount to use the new
typed signatures.
In `@app/components/ModelSelector/checkCodexStatus.ts`:
- Around line 5-9: The current module-level _cache object (named _cache) stores
the authenticated flag permanently causing stale state; change checkCodexStatus
to stop persisting authenticated in _cache (only cache installed and version)
and compute authenticated on each call, or alternatively add an explicit
invalidate function (e.g., export function invalidateCodexCache()) or implement
a TTL timestamp on _cache so authenticated is rechecked after expiry; update
references to _cache and the checkCodexStatus function to use the new
shape/invalidator and ensure any callers can trigger invalidateCodexCache() when
a retry or logout flow occurs.
- Around line 39-42: The code in checkCodexStatus.ts currently treats any
non-zero versionResult.exit_code as "Codex CLI not found"; update the logic in
the checkCodexStatus function to distinguish between a missing executable and
other failures by inspecting versionResult.stderr (and any error code if
present) for common "not found" patterns (e.g., "command not found", "not
recognized", "No such file or directory") or an ENOENT-like error, and only
log/return installed: false when those patterns match; for other non-zero exits,
change the log to indicate the CLI returned an error/crashed and return
installed: true with authenticated: false (or propagate the error) so the
message is not misleading.
In `@app/components/tools/CodexToolHandler.tsx`:
- Around line 19-22: The CodexToolHandlerProps currently types part as any,
losing type safety; define a specific interface or union (e.g., CodexToolPart or
CodexToolPartUnion) that lists the expected fields used by the component (for
example id, type, content, metadata, status, etc.), replace the any with that
new type in CodexToolHandlerProps, update the CodexToolHandler component
signature and any internal usages to match the new fields, and export/import the
type where needed so IDE and TypeScript checks verify correct usage across the
codebase.
In `@app/contexts/GlobalState.tsx`:
- Around line 261-266: The useCallback for setSelectedModelState lists chatMode
in its dependency array but the callback only calls setSelectedModelRaw; remove
chatMode and include the actual dependency (setSelectedModelRaw) instead. Update
the dependency array for setSelectedModelState to [setSelectedModelRaw] (or []
only if setSelectedModelRaw is guaranteed stable) so React hooks linting is
satisfied and the callback dependencies are correct.
In `@app/hooks/useChatHandlers.ts`:
- Around line 247-259: Pre-save failures are currently swallowed in
useChatHandlers.ts (inside the isCodexLocal branch) so streaming continues while
the chat might not be persisted; update the catch block around
saveLocalChatMutation to surface a user-visible warning (e.g., call your
toast/notification helper like showToast or toast.error) that includes a short,
friendly message plus the error detail, and keep the current behavior of
continuing the stream; optionally add a lightweight retry/enqueue-for-later flag
in the same block so the chat can be re-saved later if necessary.
In `@lib/chat/__tests__/chat-processor.test.ts`:
- Around line 184-209: Add a unit test for selectModel that asserts it throws
when given disallowed local identifiers: call selectModel("agent" or
"agent-long", any paid tier like "pro", with "codex-local" and with a namespaced
variant like "codex-local:1") and expect an error to be thrown; reference the
selectModel function in the new test so it explicitly verifies the server-side
guard rejects "codex-local" and "codex-local:*" model identifiers.
In `@lib/local-providers/codex-transport.ts`:
- Around line 620-625: Wrap the body of rpcNotify in a try/catch so Tauri invoke
failures do not propagate to callers: inside the rpcNotify method (which builds
msg and calls invoke("codex_rpc_send", { message: msg })), catch any error from
the await invoke call and handle it silently or log it (e.g., console.error or
the existing logger) instead of rethrowing; this preserves fire-and-forget
semantics while keeping the descriptive console.log("[CodexTransport] →
(notify)", method) behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6392032e-d6e9-4ef5-b602-bbd4907140b9
📒 Files selected for processing (28)
app/components/ChatInput/ChatInput.tsxapp/components/ChatInput/ChatInputToolbar.tsxapp/components/DiffView.tsxapp/components/MessagePartHandler.tsxapp/components/ModelSelector.tsxapp/components/ModelSelector/CostIndicator.tsxapp/components/ModelSelector/checkCodexStatus.tsapp/components/ModelSelector/constants.tsapp/components/ModelSelector/icons.tsxapp/components/chat.tsxapp/components/tools/CodexToolHandler.tsxapp/contexts/GlobalState.tsxapp/hooks/useChatHandlers.tsapp/hooks/useCodexLocal.tsapp/hooks/useCodexPersistence.tsconvex/chats.tsconvex/messages.tsconvex/schema.tslib/ai/providers.tslib/api/chat-handler.tslib/chat/__tests__/chat-processor.test.tslib/chat/chat-processor.tslib/local-providers/codex-transport.tslib/local-providers/delegating-transport.tslib/system-prompt.tslib/utils/sidebar-utils.tspackages/desktop/src-tauri/src/lib.rstypes/chat.ts
Comment on lines
+59
to
+63
| saveLocalChat({ | ||
| id: chatId, | ||
| title: "", // won't overwrite — saveLocalChat patches existing | ||
| codexThreadId: threadId, | ||
| }).catch(() => {}); |
There was a problem hiding this comment.
Silent error swallowing hides thread ID persistence failures.
The .catch(() => {}) discards errors without logging. If saveLocalChat fails, the thread ID won't persist and won't survive page reloads, but there's no indication of the failure.
📝 Proposed fix to log the error
if (threadId) {
saveLocalChat({
id: chatId,
title: "", // won't overwrite — saveLocalChat patches existing
codexThreadId: threadId,
- }).catch(() => {});
+ }).catch((err) => {
+ console.error("[CodexLocal] Failed to save thread ID:", err);
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| saveLocalChat({ | |
| id: chatId, | |
| title: "", // won't overwrite — saveLocalChat patches existing | |
| codexThreadId: threadId, | |
| }).catch(() => {}); | |
| saveLocalChat({ | |
| id: chatId, | |
| title: "", // won't overwrite — saveLocalChat patches existing | |
| codexThreadId: threadId, | |
| }).catch((err) => { | |
| console.error("[CodexLocal] Failed to save thread ID:", err); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/hooks/useCodexPersistence.ts` around lines 59 - 63, Replace the silent
swallow of errors on saveLocalChat with error logging: in the block that calls
saveLocalChat({ id: chatId, title: "", codexThreadId: threadId }).catch(() =>
{}), change the catch to accept the error and log it (include chatId and
threadId for context) using the existing logger or console.error so failures to
persist the thread ID are visible; ensure the call still does not throw but
surfaces the error for debugging.
Comment on lines
+208
to
+221
| // Deduplicate by message id | ||
| const existing = await ctx.db | ||
| .query("messages") | ||
| .withIndex("by_message_id", (q) => q.eq("id", args.id)) | ||
| .first(); | ||
| if (existing) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify chat ownership | ||
| const chatExists: boolean = await ctx.runQuery( | ||
| internal.messages.verifyChatOwnership, | ||
| { chatId: args.chatId, userId: user.subject }, | ||
| ); |
There was a problem hiding this comment.
Authorize before deduplicating global message ids.
This lookup returns null before internal.messages.verifyChatOwnership runs, so an authenticated caller can distinguish “that message id already exists” from “that chat is unauthorized or missing.” Please flip the order, or scope dedupe to the target chat, so unauthorized requests always fail closed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/messages.ts` around lines 208 - 221, The deduplication query checks a
global message id before authorization, leaking whether an id exists; move the
authorization check (internal.messages.verifyChatOwnership with { chatId:
args.chatId, userId: user.subject }) to run before the ctx.db query that checks
args.id, or alternatively restrict the dedupe to the target chat by changing the
lookup to include the chat scope (e.g., use/replace the global
withIndex("by_message_id") call with a query that also filters/equates on
args.chatId or use an index that keys by (chatId, id)), so unauthorized callers
cannot differentiate existence.
Comment on lines
+339
to
+341
| if (item.type === "fileChange") { | ||
| lastFileChangeToolCallId = item.id; | ||
| } |
There was a problem hiding this comment.
Only the last file change receives the aggregated diff update.
lastFileChangeToolCallId (line 212, 340) tracks only the most recent fileChange tool. When turn/diff/updated fires (line 491), only that last tool gets the updated diff. For turns with multiple file changes, earlier changes won't receive their diff updates.
🔧 Potential fix: track all fileChange IDs
- let lastFileChangeToolCallId: string | null = null;
+ const fileChangeToolCallIds: string[] = [];
// In item/started handler:
- if (item.type === "fileChange") {
- lastFileChangeToolCallId = item.id;
- }
+ if (item.type === "fileChange") {
+ fileChangeToolCallIds.push(item.id);
+ }
// In turn/diff/updated handler:
case "turn/diff/updated": {
const diff = params.diff;
- if (diff && lastFileChangeToolCallId) {
- safeEnqueue(controller, {
- type: "tool-output-available",
- toolCallId: lastFileChangeToolCallId,
- output: { codexItemType: "fileChange", output: "", diff },
- });
+ if (diff && fileChangeToolCallIds.length > 0) {
+ // Emit to the last one (or consider emitting to all)
+ for (const toolCallId of fileChangeToolCallIds) {
+ safeEnqueue(controller, {
+ type: "tool-output-available",
+ toolCallId,
+ output: { codexItemType: "fileChange", output: "", diff },
+ });
+ }
}
break;
}Also applies to: 491-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/local-providers/codex-transport.ts` around lines 339 - 341, The code only
records the most recent fileChange tool via lastFileChangeToolCallId, so when
the turn/diff/updated event fires only that single tool call gets the aggregated
diff; change the tracking to collect all fileChange IDs (e.g., replace
lastFileChangeToolCallId with an array/Set like fileChangeToolCallIds when
detecting item.type === "fileChange"), add each item.id to that collection, and
then in the turn/diff/updated handler iterate over fileChangeToolCallIds to
apply the updated diff to every corresponding tool call (and clear the
collection after applying); update any references to lastFileChangeToolCallId
accordingly so earlier fileChange tool calls receive their diffs too.
Add REST endpoints on Tauri's cmd server that proxy notes CRUD to Convex HTTP API, enabling Codex CLI to manage user notes via curl. Includes system prompt instructions, frontend wiring for cmd server info and user ID sync, and review fixes for error handling, security, and UTF-8 correctness.
- Abort send when Codex sidecar fails to start (ensureSidecar check) - Surface toast warning when local chat pre-save fails - Wrap rpcNotify in try/catch to prevent unhandled errors - Add 30s TTL to Codex auth cache to prevent stale state - Broaden message comparison to detect content updates
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/local-providers/codex-transport.ts`:
- Around line 84-95: The transport currently caches initialized = true forever,
causing initialize() to be skipped after the backend (codex app-server /
sidecar) restarts; update the lifecycle handling so initialized is reset when
the sidecar lifecycle ends or when codex_rpc_send fails: tie initialized to the
sidecar via the eventUnlisten cleanup (set this.initialized = false inside the
unlisten/teardown callback) and also clear initialized in the codex_rpc_send
failure path inside sendMessages()/rpc calls (catch/reject handler should set
this.initialized = false and optionally clear threadIds/rpc state); apply the
same reset behavior to the other initialization flag usage around the
sendMessages/turn/start flow referenced in the 177-203 region so that
initialize() is retried after backend restarts.
In `@packages/desktop/src-tauri/src/lib.rs`:
- Around line 61-68: The set_convex_config desktop command is storing and
exposing a Convex service-role key (see set_convex_config and
convex_service_key_lock), which must not be shipped in the client; remove the
service_key parameter and stop writing to convex_service_key_lock in this
function and any other client-side handlers (also remove usage around lines
noted) and instead send only non-secret config (e.g., url and user_id) from the
client; move service-key usage to a server-owned proxy or issue short-lived
scoped credentials from the server, and update any callers of set_convex_config
to stop passing HACKERAI_SERVICE_KEY over IPC.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bec06278-b258-4c90-9d70-15d22a017a3d
⛔ Files ignored due to path filters (1)
packages/desktop/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/components/ModelSelector/checkCodexStatus.tsapp/components/chat.tsxapp/hooks/useChatHandlers.tsapp/hooks/useTauri.tslib/local-providers/codex-transport.tslib/system-prompt.tspackages/desktop/src-tauri/Cargo.tomlpackages/desktop/src-tauri/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- packages/desktop/src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/system-prompt.ts
- app/components/ModelSelector/checkCodexStatus.ts
- app/hooks/useChatHandlers.ts
- app/components/chat.tsx
Comment on lines
+84
to
+95
| private initialized = false; | ||
| private threadIds = new Map<string, string>(); | ||
| private currentModel: string | undefined; | ||
| private promptData: LocalPromptData = {}; | ||
| private rpcId = 0; | ||
| private pendingRequests = new Map< | ||
| number, | ||
| { resolve: (result: any) => void; reject: (error: any) => void } | ||
| >(); | ||
| private eventUnlisten: (() => void) | null = null; | ||
| private notificationHandler: ((method: string, params: any) => void) | null = | ||
| null; |
There was a problem hiding this comment.
Reset the handshake when codex app-server restarts.
initialized is cached on the transport instance forever. After the Rust side clears state and starts a fresh app-server, the next sendMessages() skips initialize and goes straight to turn/start, so crash recovery can fail until the page reloads. Please tie this flag to the sidecar lifecycle, or clear it whenever the backend reports a restart / codex_rpc_send fails.
Also applies to: 177-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/local-providers/codex-transport.ts` around lines 84 - 95, The transport
currently caches initialized = true forever, causing initialize() to be skipped
after the backend (codex app-server / sidecar) restarts; update the lifecycle
handling so initialized is reset when the sidecar lifecycle ends or when
codex_rpc_send fails: tie initialized to the sidecar via the eventUnlisten
cleanup (set this.initialized = false inside the unlisten/teardown callback) and
also clear initialized in the codex_rpc_send failure path inside
sendMessages()/rpc calls (catch/reject handler should set this.initialized =
false and optionally clear threadIds/rpc state); apply the same reset behavior
to the other initialization flag usage around the sendMessages/turn/start flow
referenced in the 177-203 region so that initialize() is retried after backend
restarts.
Service keys cannot be embedded in desktop clients running on untrusted machines. Switch to passing the user's own Convex auth token from the frontend, using Authorization: Bearer header for HTTP API calls. - Add createUserNote, updateUserNote, searchUserNotes Convex functions - Rust backend uses auth token instead of service key - Frontend syncs WorkOS access token to Tauri every 30s - Remove all HACKERAI_SERVICE_KEY / ForBackend function references
When notes are disabled in Settings > Personalization > Notes: - System prompt shows disabled message instead of API docs - Notes content is not injected into user messages - REST endpoints return 403 with a clear message - Setting syncs to Tauri backend every 30s for mid-conversation changes
…token caching - Randomize heredoc delimiter in saveFileToLocal to prevent command injection via crafted content (security fix) - Remove category from NoteUpdateRequest — updateUserNote doesn't accept it - Default NOTES_ENABLED to false until frontend syncs user preference - Cache token to skip redundant setConvexAuth IPC calls
There was a problem hiding this comment.
🧹 Nitpick comments (7)
convex/notes.ts (2)
784-785: Consider usingidentity.tokenIdentifierinstead ofidentity.subject.The coding guidelines recommend preferring
identity.tokenIdentifieroveridentity.subjectfor auth-linked database lookups. However, I see the existing codebase (lines 567, 634, 641, 690) consistently usesidentity.subjectfor theuser_idfield.If this is an intentional pattern for this codebase (e.g.,
user_idis defined as the subject claim), then this is fine and consistent. Otherwise, consider migrating totokenIdentifierfor stronger identity binding.As per coding guidelines: "In Convex
UserIdentity, preferidentity.tokenIdentifieroveridentity.subjectfor auth-linked database lookups or ownership checks."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes.ts` around lines 784 - 785, The insert into notes uses identity.subject for user_id (see ctx.db.insert("notes", ...) and surrounding uses of identity.subject); update this to use identity.tokenIdentifier for auth-linked DB lookups by replacing identity.subject with identity.tokenIdentifier in the notes insertion (and consider aligning other occurrences—e.g., the other identity.subject usages at the same module—to maintain consistency), or explicitly document/confirm that user_id is intended to store the subject if you choose to leave it unchanged.
988-1000: Search results are unbounded — consider adding.take(n)limit.The search query collects all matching notes without a limit. While notes are user-scoped and typically won't be massive, the coding guidelines recommend returning bounded collections. Consider adding
.take(100)or similar to prevent unexpectedly large result sets.Suggested change
notes = await ctx.db .query("notes") .withSearchIndex("search_notes", (q) => { let searchQuery = q .search("content", args.search) .eq("user_id", identity.subject); if (args.category) { searchQuery = searchQuery.eq("category", args.category); } return searchQuery; }) - .collect(); + .take(100);As per coding guidelines: "Always return a bounded collection from queries using
.take()or pagination instead of.collect()unless explicitly returning all results."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/notes.ts` around lines 988 - 1000, The search branch currently calls ctx.db.query("notes").withSearchIndex("search_notes", ...) and ends with .collect(), returning an unbounded result set; change it to return a bounded collection by replacing .collect() with a .take(n) (e.g., .take(100)) or implement pagination, ensuring the withSearchIndex callback (the searchQuery that uses .search("content", args.search).eq("user_id", identity.subject) and optional .eq("category", args.category)) remains intact and the final chain uses .take(...) instead of .collect().packages/desktop/src-tauri/src/lib.rs (3)
100-102: Include response body in error for easier debugging.When the Convex API returns a non-success status, the error message only includes the status code, not the response body. Including relevant error details would help diagnose issues.
Suggested change
if !status.is_success() { - return Err(format!("Convex error ({})", status)); + return Err(format!("Convex error ({}): {}", status, text.chars().take(200).collect::<String>())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src-tauri/src/lib.rs` around lines 100 - 102, When the Convex API returns a non-success status the code only returns Err(format!("Convex error ({})", status)); — update the error path to include the response body text for diagnostics: read the response body (e.g., via response.text() / .bytes() depending on the HTTP client) into a string before returning, and include that string alongside the status in the returned Err. Locate the place using the status variable (and the HTTP response/response body variable in the same scope) and append the body content to the error message for clearer debugging.
88-95: Consider reusing the reqwest client for connection pooling.A new
reqwest::Clientis created for each Convex API call. For better performance (connection pooling, fewer allocations), consider storing a static client instance.Suggested optimization
+static REQWEST_CLIENT: std::sync::OnceLock<reqwest::Client> = std::sync::OnceLock::new(); + +fn get_reqwest_client() -> &'static reqwest::Client { + REQWEST_CLIENT.get_or_init(|| reqwest::Client::new()) +} + async fn call_convex_function(...) -> Result<String, String> { // ... - let client = reqwest::Client::new(); + let client = get_reqwest_client(); let resp = client.post(&endpoint) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src-tauri/src/lib.rs` around lines 88 - 95, The code creates a new reqwest::Client per call (let client = reqwest::Client::new();), which prevents connection pooling; replace that local client instantiation with a single shared static client (e.g., a once_cell::sync::Lazy or lazy_static CLIENT: reqwest::Client) and use CLIENT.post(&endpoint)... instead of client.post(...). Ensure the static client is constructed once (optionally with any desired default builder settings like timeouts or headers) and referenced where the current client and the send() call are used so all Convex API requests reuse the same connection pool.
774-793: SIGTERM may not cleanly shut down Codex — consider adding a timeout.
codex_kill()sends SIGTERM (Unix) ortaskkill /F(Windows) but doesn't wait for the process to exit. If the process doesn't respond to SIGTERM, it may become orphaned. Consider adding a brief wait followed by SIGKILL if needed.Note: Since this runs on
RunEvent::Exit, the app is about to close anyway, so orphaned processes may be acceptable. However, on Windows,taskkill /Fis a force kill, which is inconsistent with the Unix SIGTERM approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop/src-tauri/src/lib.rs` around lines 774 - 793, codex_kill currently sends SIGTERM (unix) or force-kills with taskkill /F (windows) and immediately returns; change it to send a graceful termination first, wait briefly with a small timeout and polls for process exit, then escalate to a hard kill if still running, and only then call codex_mark_stopped. Specifically, in codex_kill use CODEX_PID and the pid taken from the lock to: (1) on unix call libc::kill(pid, libc::SIGTERM), then poll for process existence (e.g., loop with short sleeps up to a total timeout like 1–3s) and if still alive call libc::kill(pid, libc::SIGKILL); (2) on windows first invoke taskkill without /F, wait/poll similarly, and if still alive invoke taskkill with /F to force; ensure the wait is bounded and non-blocking for long shutdowns and keep codex_mark_stopped() after termination handling.app/components/chat.tsx (2)
210-215: Unconditional Convex queries may run unnecessarily outside Tauri.
userCustomizationanduserNotesqueries run unconditionally on every render, even when not in the Tauri environment. Since these are only used for Codex local provider, consider conditionally skipping them when not in Tauri.Suggested optimization
+ const isTauri = isTauriEnvironment(); + // Convex queries for local provider prompt data (only fetched when in Tauri desktop) const userCustomization = useQuery( api.userCustomization.getUserCustomization, + isTauri ? undefined : "skip", ); - const userNotes = useQuery(api.notes.getUserNotes, {}); + const userNotes = useQuery( + api.notes.getUserNotes, + isTauri ? {} : "skip", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/chat.tsx` around lines 210 - 215, The two Convex queries userCustomization (api.userCustomization.getUserCustomization) and userNotes (api.notes.getUserNotes) are running unconditionally; change them to only run in the Tauri environment by gating them with a Tauri check (e.g., isTauri() or window.__TAURI__) — either call useQuery conditionally or pass an enabled/skip flag to useQuery so queries are disabled when not in Tauri, and ensure the derived refs userCustomizationRef and userNotesRef (useLatestRef) are created from the conditional query results (or null/empty fallback) to avoid using undefined data elsewhere.
503-511: 2-second suppression window may cause missed updates.The 2-second suppression (
codexSyncSuppressedUntilRef.current = Date.now() + 2000) prevents Convex sync after Codex stream finishes to avoid flicker. However, if a legitimate external update occurs within this window (e.g., another device updates a message), it will be ignored.This is likely acceptable for the Codex local provider use case since it's a single-user desktop context, but worth documenting this trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/chat.tsx` around lines 503 - 511, The fixed 2s suppression in the Codex local path (isCodexLocal(selectedModelRef.current)) can drop legitimate external Convex updates because codexSyncSuppressedUntilRef.current = Date.now() + 2000 is unconditional; change this by replacing the magic 2000 with a named constant (e.g., CODEX_SYNC_SUPPRESSION_MS) and either lower it (e.g., 500ms) or make it configurable, and add a short comment documenting the trade-off; alternatively, implement a more robust check (e.g., detect message origin/echo flag on messages in persistCodexMessages or compare message IDs/timestamps before suppressing) so external updates aren’t silently ignored while still preventing echo-induced flicker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/chat.tsx`:
- Around line 210-215: The two Convex queries userCustomization
(api.userCustomization.getUserCustomization) and userNotes
(api.notes.getUserNotes) are running unconditionally; change them to only run in
the Tauri environment by gating them with a Tauri check (e.g., isTauri() or
window.__TAURI__) — either call useQuery conditionally or pass an enabled/skip
flag to useQuery so queries are disabled when not in Tauri, and ensure the
derived refs userCustomizationRef and userNotesRef (useLatestRef) are created
from the conditional query results (or null/empty fallback) to avoid using
undefined data elsewhere.
- Around line 503-511: The fixed 2s suppression in the Codex local path
(isCodexLocal(selectedModelRef.current)) can drop legitimate external Convex
updates because codexSyncSuppressedUntilRef.current = Date.now() + 2000 is
unconditional; change this by replacing the magic 2000 with a named constant
(e.g., CODEX_SYNC_SUPPRESSION_MS) and either lower it (e.g., 500ms) or make it
configurable, and add a short comment documenting the trade-off; alternatively,
implement a more robust check (e.g., detect message origin/echo flag on messages
in persistCodexMessages or compare message IDs/timestamps before suppressing) so
external updates aren’t silently ignored while still preventing echo-induced
flicker.
In `@convex/notes.ts`:
- Around line 784-785: The insert into notes uses identity.subject for user_id
(see ctx.db.insert("notes", ...) and surrounding uses of identity.subject);
update this to use identity.tokenIdentifier for auth-linked DB lookups by
replacing identity.subject with identity.tokenIdentifier in the notes insertion
(and consider aligning other occurrences—e.g., the other identity.subject usages
at the same module—to maintain consistency), or explicitly document/confirm that
user_id is intended to store the subject if you choose to leave it unchanged.
- Around line 988-1000: The search branch currently calls
ctx.db.query("notes").withSearchIndex("search_notes", ...) and ends with
.collect(), returning an unbounded result set; change it to return a bounded
collection by replacing .collect() with a .take(n) (e.g., .take(100)) or
implement pagination, ensuring the withSearchIndex callback (the searchQuery
that uses .search("content", args.search).eq("user_id", identity.subject) and
optional .eq("category", args.category)) remains intact and the final chain uses
.take(...) instead of .collect().
In `@packages/desktop/src-tauri/src/lib.rs`:
- Around line 100-102: When the Convex API returns a non-success status the code
only returns Err(format!("Convex error ({})", status)); — update the error path
to include the response body text for diagnostics: read the response body (e.g.,
via response.text() / .bytes() depending on the HTTP client) into a string
before returning, and include that string alongside the status in the returned
Err. Locate the place using the status variable (and the HTTP response/response
body variable in the same scope) and append the body content to the error
message for clearer debugging.
- Around line 88-95: The code creates a new reqwest::Client per call (let client
= reqwest::Client::new();), which prevents connection pooling; replace that
local client instantiation with a single shared static client (e.g., a
once_cell::sync::Lazy or lazy_static CLIENT: reqwest::Client) and use
CLIENT.post(&endpoint)... instead of client.post(...). Ensure the static client
is constructed once (optionally with any desired default builder settings like
timeouts or headers) and referenced where the current client and the send() call
are used so all Convex API requests reuse the same connection pool.
- Around line 774-793: codex_kill currently sends SIGTERM (unix) or force-kills
with taskkill /F (windows) and immediately returns; change it to send a graceful
termination first, wait briefly with a small timeout and polls for process exit,
then escalate to a hard kill if still running, and only then call
codex_mark_stopped. Specifically, in codex_kill use CODEX_PID and the pid taken
from the lock to: (1) on unix call libc::kill(pid, libc::SIGTERM), then poll for
process existence (e.g., loop with short sleeps up to a total timeout like 1–3s)
and if still alive call libc::kill(pid, libc::SIGKILL); (2) on windows first
invoke taskkill without /F, wait/poll similarly, and if still alive invoke
taskkill with /F to force; ensure the wait is bounded and non-blocking for long
shutdowns and keep codex_mark_stopped() after termination handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebf93050-cfda-4c0d-a74e-2da8eae9a9e3
📒 Files selected for processing (6)
__mocks__/workos.tsapp/components/chat.tsxapp/hooks/useTauri.tsconvex/notes.tslib/system-prompt.tspackages/desktop/src-tauri/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- mocks/workos.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/hooks/useTauri.ts
- Remove length-based key from DiffView to prevent streaming remounts - Add type="button" to raw buttons in ModelSelector - Add part.input/part.type to CodexToolHandler arePropsEqual - Reset transport handshake state on Codex process restart - Emit codex-server-exit event so frontend detects crashes - Fix persistCodexMessages return value on failure - Persist selectedModel on chat upsert path
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
app/hooks/useCodexPersistence.ts (1)
57-63:⚠️ Potential issue | 🟡 MinorDon't hide thread-ID persistence failures.
If
saveLocalChatfails here, reload/resume loses the Codex thread with no diagnostic.📝 Proposed fix
if (threadId) { saveLocalChat({ id: chatId, title: "", // won't overwrite — saveLocalChat patches existing codexThreadId: threadId, - }).catch(() => {}); + }).catch((error) => { + console.error("[CodexLocal] Failed to save thread ID", { + chatId, + threadId, + error, + }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useCodexPersistence.ts` around lines 57 - 63, The current code calls codexTransport.getThreadId(chatId) and then invokes saveLocalChat({...}).catch(() => {}) which silently swallows persistence failures; change this to surface errors by removing the empty catch and instead handle errors by logging them with the app logger (e.g., processLogger.error or the module's logger) including the chatId and threadId, and/or rethrowing so the caller can react; update the call site around saveLocalChat in useCodexPersistence.ts to await/saveLocalChat(...).catch(err => { processLogger.error("Failed to persist Codex thread for chat", { chatId, threadId, err }); throw err; }) or similar so failures are not hidden.lib/local-providers/codex-transport.ts (1)
239-240:⚠️ Potential issue | 🟡 MinorSend aggregated diffs to every
fileChange, not just the last one.This still keeps only one
lastFileChangeToolCallId, so a multi-file turn replaysturn/diff/updatedinto the final change only. Earlier file changes never receive the aggregated diff.🔧 Proposed fix
- let lastFileChangeToolCallId: string | null = null; + const fileChangeToolCallIds = new Set<string>(); ... if (item.type === "fileChange") { - lastFileChangeToolCallId = item.id; + fileChangeToolCallIds.add(item.id); } ... - if (diff && lastFileChangeToolCallId) { - // Re-emit tool output with the full git diff - safeEnqueue(controller, { - type: "tool-output-available", - toolCallId: lastFileChangeToolCallId, - output: { - codexItemType: "fileChange", - output: "", - diff, - }, - }); + if (diff) { + for (const toolCallId of fileChangeToolCallIds) { + safeEnqueue(controller, { + type: "tool-output-available", + toolCallId, + output: { + codexItemType: "fileChange", + output: "", + diff, + }, + }); + } }Also applies to: 367-369, 518-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/local-providers/codex-transport.ts` around lines 239 - 240, The code only stores a single lastFileChangeToolCallId so aggregated "turn/diff/updated" is sent to the final file change only; replace this single-id usage with a collection (e.g., lastFileChangeToolCallIds: string[] or Set<string>) and update all places that set/read lastFileChangeToolCallId to push/add the toolCallId to the collection, then when emitting the aggregated diff (where you currently reference lastFileChangeToolCallId) iterate the collection and send the "turn/diff/updated" update for each id; finally, clear the collection at the end of the multi-file turn. Apply the same replacement in the other occurrences mentioned (around the blocks currently using lastFileChangeToolCallId).
🧹 Nitpick comments (2)
app/components/ModelSelector.tsx (1)
265-284: Hover-based submenu may close prematurely due to portal gap.The desktop Popover submenu relies on
onMouseEnter/onMouseLeavehandlers on both the wrapper div andPopoverContent. Since Radix rendersPopoverContentin a portal (outside the wrapper div's DOM subtree), moving the cursor through the 8pxsideOffsetgap will triggeronMouseLeaveon the wrapper beforeonMouseEnteron the content, potentially closing the submenu.Consider adding a small delay before closing to bridge the gap:
Example approach with close delay
const closeTimeoutRef = useRef<ReturnType<typeof setTimeout>>(); const handleMouseEnter = () => { clearTimeout(closeTimeoutRef.current); setSubOpen(true); }; const handleMouseLeave = () => { closeTimeoutRef.current = setTimeout(() => setSubOpen(false), 100); }; // Use handleMouseEnter/handleMouseLeave on both wrapper and PopoverContent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ModelSelector.tsx` around lines 265 - 284, The hover-based submenu can close when the cursor crosses the portal gap; modify the handlers around Popover/PopoverContent to debounce closing by storing a timeout ref (e.g., closeTimeoutRef), call clearTimeout(closeTimeoutRef.current) and setSubOpen(true) on mouse enter, and on mouse leave start a short timeout (≈100ms) that calls setSubOpen(false); attach these enter/leave handlers to both the wrapper div and PopoverContent (which is rendered in a portal) so the brief delay bridges the sideOffset gap and prevents premature closing of subOpen.app/hooks/useCodexPersistence.ts (1)
39-54: Avoid replaying the full transcript into Convex on every finish.This re-sends every prior user/assistant message each time a Codex turn completes. As chats grow, write volume becomes quadratic and
Promise.all(...)can fan out a large burst of mutations. Persist only new/changed messages, or batch them in one mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/useCodexPersistence.ts` around lines 39 - 54, The current Promise.all over messages in useCodexPersistence.ts re-sends the entire transcript on each Codex turn (messages.filter(...).map(...).saveLocalMessage), causing quadratic writes; change this to only persist new/changed messages or send them in a single batched mutation: add logic to detect which messages are new/dirty (e.g., track lastPersistedMessageId or a message.persisted flag) and filter to that subset, then replace the per-message saveLocalMessage calls with a single batch-saving API (e.g., implement batchSaveLocalMessages that accepts an array or a Convex mutation to upsert many messages at once), updating references to saveLocalMessage and selectedModelRef.current usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/tools/CodexToolHandler.tsx`:
- Line 227: The UI marks items as clickable unconditionally (isClickable={true})
even though handleOpenInSidebar returns early when sidebarContent is null,
creating a dead-click; change the isClickable prop to reflect whether
sidebarContent exists (e.g., isClickable={Boolean(sidebarContent)}) and also
guard or conditionally attach the click handler (handleOpenInSidebar) so clicks
are only possible when sidebarContent is non-null, using the same sidebarContent
check to keep behavior and affordance consistent.
In `@lib/local-providers/codex-transport.ts`:
- Around line 155-185: startListening() can be entered concurrently causing
duplicate Tauri subscriptions; add a memoization guard (e.g., a
this.startListeningPromise or temporarily set this.eventUnlisten to a sentinel)
at the top of startListening() and assign it before awaiting
import("@tauri-apps/api/event").listen so subsequent callers await the same
promise instead of creating another listener; ensure the promise is cleared or
the sentinel replaced with the real unlisten function on success and
rejected/cleared on error. This change should be applied around the
startListening() logic that calls listen and sets this.eventUnlisten (also
mirror the same memoization pattern for the analogous code referenced at lines
200-201).
- Around line 203-229: The initialize/start sequence can still trigger a hidden
Codex turn if a fast Stop happens before turnId is set; ensure
cancellation/closed-stream is checked and an interrupt sent even if turnId is
not yet assigned by either (a) setting a provisional turnId before calling
rpcRequest("initialize")/thread start so interrupts can reference it, or (b)
guarding the rpcNotify("turn/start")/rpcNotify("turn/started") calls with a
check for stream closure/cancellation and calling errorStream or sending an
interrupt notification immediately when cancelled; modify the logic around
this.initialized, this.turnId, rpcRequest("initialize"), rpcNotify("turn/start")
and rpcNotify("turn/started") so no backend turn can be started when the local
stream is closed (also apply same fix to the other similar blocks noted).
---
Duplicate comments:
In `@app/hooks/useCodexPersistence.ts`:
- Around line 57-63: The current code calls codexTransport.getThreadId(chatId)
and then invokes saveLocalChat({...}).catch(() => {}) which silently swallows
persistence failures; change this to surface errors by removing the empty catch
and instead handle errors by logging them with the app logger (e.g.,
processLogger.error or the module's logger) including the chatId and threadId,
and/or rethrowing so the caller can react; update the call site around
saveLocalChat in useCodexPersistence.ts to await/saveLocalChat(...).catch(err =>
{ processLogger.error("Failed to persist Codex thread for chat", { chatId,
threadId, err }); throw err; }) or similar so failures are not hidden.
In `@lib/local-providers/codex-transport.ts`:
- Around line 239-240: The code only stores a single lastFileChangeToolCallId so
aggregated "turn/diff/updated" is sent to the final file change only; replace
this single-id usage with a collection (e.g., lastFileChangeToolCallIds:
string[] or Set<string>) and update all places that set/read
lastFileChangeToolCallId to push/add the toolCallId to the collection, then when
emitting the aggregated diff (where you currently reference
lastFileChangeToolCallId) iterate the collection and send the
"turn/diff/updated" update for each id; finally, clear the collection at the end
of the multi-file turn. Apply the same replacement in the other occurrences
mentioned (around the blocks currently using lastFileChangeToolCallId).
---
Nitpick comments:
In `@app/components/ModelSelector.tsx`:
- Around line 265-284: The hover-based submenu can close when the cursor crosses
the portal gap; modify the handlers around Popover/PopoverContent to debounce
closing by storing a timeout ref (e.g., closeTimeoutRef), call
clearTimeout(closeTimeoutRef.current) and setSubOpen(true) on mouse enter, and
on mouse leave start a short timeout (≈100ms) that calls setSubOpen(false);
attach these enter/leave handlers to both the wrapper div and PopoverContent
(which is rendered in a portal) so the brief delay bridges the sideOffset gap
and prevents premature closing of subOpen.
In `@app/hooks/useCodexPersistence.ts`:
- Around line 39-54: The current Promise.all over messages in
useCodexPersistence.ts re-sends the entire transcript on each Codex turn
(messages.filter(...).map(...).saveLocalMessage), causing quadratic writes;
change this to only persist new/changed messages or send them in a single
batched mutation: add logic to detect which messages are new/dirty (e.g., track
lastPersistedMessageId or a message.persisted flag) and filter to that subset,
then replace the per-message saveLocalMessage calls with a single batch-saving
API (e.g., implement batchSaveLocalMessages that accepts an array or a Convex
mutation to upsert many messages at once), updating references to
saveLocalMessage and selectedModelRef.current usage accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22600202-f90d-4bf4-8bbd-c5d9f6e8af04
📒 Files selected for processing (8)
app/components/DiffView.tsxapp/components/ModelSelector.tsxapp/components/tools/CodexToolHandler.tsxapp/hooks/useCodexLocal.tsapp/hooks/useCodexPersistence.tsconvex/chats.tslib/local-providers/codex-transport.tspackages/desktop/src-tauri/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- convex/chats.ts
- app/hooks/useCodexLocal.ts
- packages/desktop/src-tauri/src/lib.rs
Comment on lines
+56
to
+63
| if ( | ||
| line.startsWith("diff --git") || | ||
| line.startsWith("index ") || | ||
| line.startsWith("---") || | ||
| line.startsWith("+++") || | ||
| line.startsWith("new file") || | ||
| line.startsWith("deleted file") | ||
| ) { |
There was a problem hiding this comment.
parseGitDiff drops valid hunk content that starts with ---/+++
The header filter runs before hunk-context handling and skips any line starting with --- or +++. In hunks, those can be legitimate removed/added content (e.g. ---flag, +++value), so parsed output can be corrupted.
🐛 Suggested fix
- if (
- line.startsWith("diff --git") ||
- line.startsWith("index ") ||
- line.startsWith("---") ||
- line.startsWith("+++") ||
- line.startsWith("new file") ||
- line.startsWith("deleted file")
- ) {
+ if (
+ !inHunk &&
+ (line.startsWith("diff --git") ||
+ line.startsWith("index ") ||
+ line.startsWith("--- ") ||
+ line.startsWith("+++ ") ||
+ line.startsWith("new file ") ||
+ line.startsWith("deleted file "))
+ ) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| line.startsWith("diff --git") || | |
| line.startsWith("index ") || | |
| line.startsWith("---") || | |
| line.startsWith("+++") || | |
| line.startsWith("new file") || | |
| line.startsWith("deleted file") | |
| ) { | |
| if ( | |
| !inHunk && | |
| (line.startsWith("diff --git") || | |
| line.startsWith("index ") || | |
| line.startsWith("--- ") || | |
| line.startsWith("+++ ") || | |
| line.startsWith("new file ") || | |
| line.startsWith("deleted file ")) | |
| ) { |
| action={isExecuting ? display.runningAction : display.doneAction} | ||
| target={display.target} | ||
| isShimmer={isExecuting} | ||
| isClickable={true} |
There was a problem hiding this comment.
Clickable affordance should match actual open behavior
Line 227 always sets isClickable={true}, but handleOpenInSidebar exits when sidebarContent is null. That creates dead-click UI in fallback/empty-content cases.
♻️ Suggested fix
- isClickable={true}
+ isClickable={sidebarContent !== null}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| isClickable={true} | |
| isClickable={sidebarContent !== null} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/tools/CodexToolHandler.tsx` at line 227, The UI marks items as
clickable unconditionally (isClickable={true}) even though handleOpenInSidebar
returns early when sidebarContent is null, creating a dead-click; change the
isClickable prop to reflect whether sidebarContent exists (e.g.,
isClickable={Boolean(sidebarContent)}) and also guard or conditionally attach
the click handler (handleOpenInSidebar) so clicks are only possible when
sidebarContent is non-null, using the same sidebarContent check to keep behavior
and affordance consistent.
Comment on lines
+155
to
+185
| async startListening(): Promise<void> { | ||
| if (this.eventUnlisten) return; | ||
|
|
||
| const { listen } = await import("@tauri-apps/api/event"); | ||
| this.eventUnlisten = await listen<string>("codex-rpc-event", (event) => { | ||
| try { | ||
| const msg = JSON.parse(event.payload); | ||
|
|
||
| // Response to a request (has id field) | ||
| if ("id" in msg && this.pendingRequests.has(msg.id)) { | ||
| const pending = this.pendingRequests.get(msg.id)!; | ||
| this.pendingRequests.delete(msg.id); | ||
| if (msg.error) { | ||
| pending.reject(new Error(msg.error.message || "RPC error")); | ||
| } else { | ||
| pending.resolve(msg.result); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Notification (no id field) — forward to current handler | ||
| if (msg.method && this.notificationHandler) { | ||
| this.notificationHandler(msg.method, msg.params || {}); | ||
| } | ||
| } catch (err) { | ||
| console.warn("[CodexTransport] Failed to parse event:", err); | ||
| } | ||
| }); | ||
|
|
||
| console.log("[CodexTransport] Listening for Tauri events"); | ||
| } |
There was a problem hiding this comment.
Memoize startListening() to prevent duplicate Tauri subscriptions.
Line 156 only guards the steady state. If two callers enter startListening() before listen() resolves, both register a handler and every notification gets processed twice.
🔧 Proposed fix
export class CodexLocalTransport {
+ private listeningPromise: Promise<void> | null = null;
...
reset() {
...
+ this.listeningPromise = null;
}
async startListening(): Promise<void> {
if (this.eventUnlisten) return;
+ if (this.listeningPromise) return this.listeningPromise;
- const { listen } = await import("@tauri-apps/api/event");
- this.eventUnlisten = await listen<string>("codex-rpc-event", (event) => {
- try {
- const msg = JSON.parse(event.payload);
- // existing handler body
- } catch (err) {
- console.warn("[CodexTransport] Failed to parse event:", err);
- }
- });
-
- console.log("[CodexTransport] Listening for Tauri events");
+ this.listeningPromise = (async () => {
+ const { listen } = await import("@tauri-apps/api/event");
+ this.eventUnlisten = await listen<string>("codex-rpc-event", (event) => {
+ try {
+ const msg = JSON.parse(event.payload);
+ // existing handler body
+ } catch (err) {
+ console.warn("[CodexTransport] Failed to parse event:", err);
+ }
+ });
+
+ console.log("[CodexTransport] Listening for Tauri events");
+ })();
+
+ try {
+ await this.listeningPromise;
+ } finally {
+ this.listeningPromise = null;
+ }
}Also applies to: 200-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/local-providers/codex-transport.ts` around lines 155 - 185,
startListening() can be entered concurrently causing duplicate Tauri
subscriptions; add a memoization guard (e.g., a this.startListeningPromise or
temporarily set this.eventUnlisten to a sentinel) at the top of startListening()
and assign it before awaiting import("@tauri-apps/api/event").listen so
subsequent callers await the same promise instead of creating another listener;
ensure the promise is cleared or the sentinel replaced with the real unlisten
function on success and rejected/cleared on error. This change should be applied
around the startListening() logic that calls listen and sets this.eventUnlisten
(also mirror the same memoization pattern for the analogous code referenced at
lines 200-201).
Comment on lines
+203
to
+229
| // Initialize handshake (first time only) | ||
| if (!this.initialized) { | ||
| try { | ||
| console.log("[CodexTransport] Initializing..."); | ||
| await this.rpcRequest("initialize", { | ||
| clientInfo: { | ||
| name: "hackerai", | ||
| title: "HackerAI Desktop", | ||
| version: "0.1.0", | ||
| }, | ||
| }); | ||
| this.rpcNotify("initialized", {}); | ||
| this.initialized = true; | ||
| console.log("[CodexTransport] Initialized"); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| // "Already initialized" means the process survived HMR — treat as success | ||
| if (msg.includes("Already initialized")) { | ||
| console.log( | ||
| "[CodexTransport] Already initialized (process survived reload)", | ||
| ); | ||
| this.initialized = true; | ||
| } else { | ||
| return this.errorStream(`Initialize failed: ${msg}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
A fast Stop can still launch a hidden Codex turn.
Lines 563-585 only interrupt once turnId exists. If the user cancels during initialize/thread start or before turn/started, no interrupt is sent and the code still reaches turn/start on Lines 592-596. Because the stream is already closed locally at that point, the backend can keep running commands or file changes unseen. With sandbox: "danger-full-access" on Lines 278-279, this is release-blocking.
🛑 Proposed fix
+ let abortRequested = abortSignal?.aborted ?? false;
+
// Set up notification handler for this turn
this.notificationHandler = (method, params) => {
switch (method) {
case "turn/started": {
turnId = params.turn?.id;
+ if (abortRequested && turnId) {
+ this.rpcNotify("turn/interrupt", { threadId, turnId });
+ this.notificationHandler = null;
+ safeEnqueue(controller, { type: "finish-step" });
+ safeEnqueue(controller, {
+ type: "finish",
+ finishReason: "stop",
+ });
+ safeClose(controller);
+ break;
+ }
...
}
}
};
// Handle abort
if (abortSignal) {
+ if (abortSignal.aborted) {
+ safeEnqueue(controller, { type: "finish-step" });
+ safeEnqueue(controller, {
+ type: "finish",
+ finishReason: "stop",
+ });
+ safeClose(controller);
+ return;
+ }
+
abortSignal.addEventListener(
"abort",
() => {
+ abortRequested = true;
if (turnId && threadId) {
this.rpcNotify("turn/interrupt", { threadId, turnId });
+ this.notificationHandler = null;
+ safeEnqueue(controller, { type: "finish-step" });
+ safeEnqueue(controller, {
+ type: "finish",
+ finishReason: "stop",
+ });
+ safeClose(controller);
+ return;
}
- // existing local close logic
},
{ once: true },
);
}
+ if (abortRequested) return;
+
await this.rpcRequest("turn/start", {Also applies to: 257-292, 562-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/local-providers/codex-transport.ts` around lines 203 - 229, The
initialize/start sequence can still trigger a hidden Codex turn if a fast Stop
happens before turnId is set; ensure cancellation/closed-stream is checked and
an interrupt sent even if turnId is not yet assigned by either (a) setting a
provisional turnId before calling rpcRequest("initialize")/thread start so
interrupts can reference it, or (b) guarding the
rpcNotify("turn/start")/rpcNotify("turn/started") calls with a check for stream
closure/cancellation and calling errorStream or sending an interrupt
notification immediately when cancelled; modify the logic around
this.initialized, this.turnId, rpcRequest("initialize"), rpcNotify("turn/start")
and rpcNotify("turn/started") so no backend turn can be started when the local
stream is closed (also apply same fix to the other similar blocks noted).
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.
Adds Codex (OpenAI Codex CLI) as a local provider option in the desktop app, communicating via Tauri IPC + stdio JSON-RPC. Includes fixes for:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements