Local model chat: shared helpers, banner merge, permission reset#132
Conversation
Export LOCAL_PROVIDER_LABELS and local model id parsers from modelRegistry; dedupe labels in agentChatService and aiIntegrationService. Merge CLI and local runtime banners in AgentChatPane, reset unified permission mode when harness recommendations change, and fix local placeholder sdkModelId. Settings and model options use the shared helpers. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change consolidates local model provider parsing and labeling logic from multiple desktop components into a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 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 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 review |
|
@codex review |
✅ Actions performedReview triggered.
|
|
@copilot review |
Code ReviewThe changes are clean and well-scoped. Here's my assessment: ✅ Shared helpers (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
1816-1830: Sequential setState calls are correct but could be consolidated.The two
setUnifiedPermissionModecalls in lines 1824-1829 are batched by React, so only the final value applies. The current structure makes the two-step intent clear (reset first, then apply recommendation), but could be simplified.♻️ Optional consolidation
- if (snapshot.resetUnifiedPermissionToDefault) { - setUnifiedPermissionMode(initialNativeControls.unifiedPermissionMode); - } - if (snapshot.nextUnifiedPermissionMode) { + const effectiveMode = snapshot.resetUnifiedPermissionToDefault + ? (snapshot.nextUnifiedPermissionMode ?? initialNativeControls.unifiedPermissionMode) + : snapshot.nextUnifiedPermissionMode; + if (effectiveMode) { setUnifiedPermissionMode(snapshot.nextUnifiedPermissionMode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 1816 - 1830, The applyModelSelectionSnapshot callback uses two sequential setUnifiedPermissionMode calls; consolidate them by computing the intended unifiedPermissionMode first (e.g., if snapshot.resetUnifiedPermissionToDefault then initialNativeControls.unifiedPermissionMode else snapshot.nextUnifiedPermissionMode if present else leave unchanged) and then call setUnifiedPermissionMode exactly once after setModelId and setReasoningEffort; update the applyModelSelectionSnapshot function to determine the final value and invoke setUnifiedPermissionMode(singleValue) instead of calling it twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1816-1830: The applyModelSelectionSnapshot callback uses two
sequential setUnifiedPermissionMode calls; consolidate them by computing the
intended unifiedPermissionMode first (e.g., if
snapshot.resetUnifiedPermissionToDefault then
initialNativeControls.unifiedPermissionMode else
snapshot.nextUnifiedPermissionMode if present else leave unchanged) and then
call setUnifiedPermissionMode exactly once after setModelId and
setReasoningEffort; update the applyModelSelectionSnapshot function to determine
the final value and invoke setUnifiedPermissionMode(singleValue) instead of
calling it twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54e2d831-0e43-4133-b681-27b58d0f3bdc
📒 Files selected for processing (7)
apps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/shared/UnifiedModelSelector.tsxapps/desktop/src/renderer/lib/modelOptions.tsapps/desktop/src/shared/modelRegistry.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ebb03faf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nextReasoningEffort, | ||
| nextUnifiedPermissionMode: recommendedUnifiedPermissionModeForModel(nextDesc), | ||
| nextUnifiedPermissionMode: nextRec, | ||
| resetUnifiedPermissionToDefault: shouldResetUnifiedPermissionForModelSwitch(previousDesc, nextDesc), |
There was a problem hiding this comment.
resetUnifiedPermissionToDefault is computed from nextDesc, which comes from getModelById(nextModelId). For runtime-discovered local IDs that are not present in the renderer registry (they are represented as selector placeholders), nextDesc is null, so a switch from a guarded local model (plan) to one of these discovered models is treated as plan -> null and gets reset to the surface default. That downgrades the session out of plan mode and the downgraded mode is then sent in updateSession, so users can lose guarded local safety just by selecting a discovered local model.
Useful? React with 👍 / 👎.
Add getModelDescriptorForPermissionMode for harness decisions when getModelById is undefined (e.g. ollama/auto). Use it in AgentChatPane for prevModelRef, model switch snapshots, and session create. Consolidate applyModelSelectionSnapshot to a single setUnifiedPermissionMode call. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
* Polish local model chat UI and share provider helpers Export LOCAL_PROVIDER_LABELS and local model id parsers from modelRegistry; dedupe labels in agentChatService and aiIntegrationService. Merge CLI and local runtime banners in AgentChatPane, reset unified permission mode when harness recommendations change, and fix local placeholder sdkModelId. Settings and model options use the shared helpers. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> * Address PR review: local permission descriptors and unified mode setter Add getModelDescriptorForPermissionMode for harness decisions when getModelById is undefined (e.g. ollama/auto). Use it in AgentChatPane for prevModelRef, model switch snapshots, and session create. Consolidate applyModelSelectionSnapshot to a single setUnifiedPermissionMode call. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
* Polish local model chat UI and share provider helpers Export LOCAL_PROVIDER_LABELS and local model id parsers from modelRegistry; dedupe labels in agentChatService and aiIntegrationService. Merge CLI and local runtime banners in AgentChatPane, reset unified permission mode when harness recommendations change, and fix local placeholder sdkModelId. Settings and model options use the shared helpers. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> * Address PR review: local permission descriptors and unified mode setter Add getModelDescriptorForPermissionMode for harness decisions when getModelById is undefined (e.g. ollama/auto). Use it in AgentChatPane for prevModelRef, model switch snapshots, and session create. Consolidate applyModelSelectionSnapshot to a single setUnifiedPermissionMode call. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
* Polish local model chat UI and share provider helpers Export LOCAL_PROVIDER_LABELS and local model id parsers from modelRegistry; dedupe labels in agentChatService and aiIntegrationService. Merge CLI and local runtime banners in AgentChatPane, reset unified permission mode when harness recommendations change, and fix local placeholder sdkModelId. Settings and model options use the shared helpers. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> * Address PR review: local permission descriptors and unified mode setter Add getModelDescriptorForPermissionMode for harness decisions when getModelById is undefined (e.g. ollama/auto). Use it in AgentChatPane for prevModelRef, model switch snapshots, and session create. Consolidate applyModelSelectionSnapshot to a single setUnifiedPermissionMode call. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
* Polish local model chat UI and share provider helpers Export LOCAL_PROVIDER_LABELS and local model id parsers from modelRegistry; dedupe labels in agentChatService and aiIntegrationService. Merge CLI and local runtime banners in AgentChatPane, reset unified permission mode when harness recommendations change, and fix local placeholder sdkModelId. Settings and model options use the shared helpers. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> * Address PR review: local permission descriptors and unified mode setter Add getModelDescriptorForPermissionMode for harness decisions when getModelById is undefined (e.g. ollama/auto). Use it in AgentChatPane for prevModelRef, model switch snapshots, and session create. Consolidate applyModelSelectionSnapshot to a single setUnifiedPermissionMode call. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
Summary
Follow-up to the local AI runtime work: addresses the audit items for local model chat UX and code hygiene.
Changes
LOCAL_PROVIDER_LABELS,parseLocalProviderFromModelId,getLocalModelIdTail, andisLocalProviderFamilyfrommodelRegistry; dedupe labels inagentChatService/aiIntegrationService.getModelDescriptorForPermissionMode— WhengetModelByIdis undefined (e.g.ollama/auto), still returns a guarded local descriptor so harness / unified permission logic matches main (addresses Codex P1 on PR review).setUnifiedPermissionModeinapplyModelSelectionSnapshot(CodeRabbit nitpick). Reset + recommendation combined as(nextRec ?? surfaceDefault).sdkModelIdtail.Post-review (commit
be3a62e6)getModelDescriptorForPermissionModethroughprevModelDescRef,buildModelSelectionSnapshot, andcreateSessionharness path.modelRegistry.test.tsforollama/autovs known local ids.Validation
npm --prefix apps/desktop run typecheck/lintvitest—modelRegistry.test.ts,AgentChatPane.test.ts,modelOptions.test.ts,ProvidersSection.test.tsxnpm --prefix apps/mcp-server run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Refactor