Add per-chat settings for chat mode and selected model#2544
Add per-chat settings for chat mode and selected model#2544princeaden1 wants to merge 2 commits intodyad-sh:mainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello @princeaden1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by enabling per-chat configuration for both the chat mode and the selected language model. This change allows users to tailor the behavior of each conversation independently, providing greater flexibility and a more personalized experience without affecting global application settings. The implementation includes database schema modifications, new frontend hooks, updated UI components, and backend logic to ensure these settings are consistently applied and persisted. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile OverviewGreptile SummaryThis PR implements per-chat settings for chat mode and selected model, allowing each chat to maintain its own configuration independent of global settings. When a chat is selected, the UI components ( Key Changes:
Architecture: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatModeSelector
participant useChatSettings
participant IpcClient
participant ChatHandlers
participant Database
User->>ChatModeSelector: Select chat mode
ChatModeSelector->>useChatSettings: updateChatMode(mode)
useChatSettings->>IpcClient: updateChatSettings(chatId, mode)
IpcClient->>ChatHandlers: chat:update-settings
ChatHandlers->>Database: UPDATE chats SET chat_mode
Database-->>ChatHandlers: success
ChatHandlers-->>IpcClient: void
IpcClient-->>useChatSettings: void
useChatSettings->>useChatSettings: invalidateQueries(chat settings)
useChatSettings->>useChatSettings: invalidateQueries(token count)
useChatSettings-->>ChatModeSelector: updated
Note over User,Database: When sending a message
User->>ChatModeSelector: Send message
ChatModeSelector->>ChatHandlers: chat:stream-request
ChatHandlers->>Database: SELECT chat settings
Database-->>ChatHandlers: chatMode, selectedModel
ChatHandlers->>ChatHandlers: Merge with global settings
ChatHandlers->>ChatHandlers: Use effective settings for stream
Last reviewed commit: ac87ef3 |
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Creating new chat modifies global selectedChatMode affecting other chats without per-chat overrides (src/components/ChatList.tsx:104)
In ChatList.tsx:104, when creating a new chat, the code calls updateSettings({ selectedChatMode: effectiveDefaultMode }) which modifies the global selectedChatMode setting rather than setting a per-chat mode for the new chat.
Root Cause
With the new per-chat settings feature, the global selectedChatMode serves as a fallback for chats that don't have an explicit per-chat mode set. When ChatList.tsx modifies the global setting on every new chat creation, it can inadvertently change the displayed mode for all other existing chats that rely on the global fallback.
Example scenario:
- User sets global default to "build"
- User creates Chat #1, explicitly sets it to "ask" (stored as per-chat setting)
- User creates Chat #2 - code resets global to "local-agent" (from
getEffectiveDefaultChatMode) - Now Chat #1 still shows "ask" (has per-chat override), but any chat without per-chat settings will show "local-agent" instead of the user's intended "build" default
Impact: The global chat mode setting gets overwritten every time a new chat is created, causing unexpected mode changes for chats relying on the global fallback.
View 5 additional findings in Devin Review.
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/useChatSettings.ts">
<violation number="1" location="src/hooks/useChatSettings.ts:71">
P2: `isUsingGlobalSettings` reports false when `chatSettings` is undefined (e.g., disabled or loading query), even though the hook falls back to global settings. Treat both `null` and `undefined` as “no per-chat override.”</violation>
</file>
<file name="src/ipc/handlers/chat_handlers.ts">
<violation number="1" location="src/ipc/handlers/chat_handlers.ts:207">
P2: Guard against empty updates. If no fields are provided, return early (or throw a validation error) before calling update; otherwise this can execute an UPDATE with no SET columns and fail at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| // Check if using global settings (no per-chat override) | ||
| isUsingGlobalSettings: { | ||
| chatMode: chatSettings?.chatMode === null, |
There was a problem hiding this comment.
P2: isUsingGlobalSettings reports false when chatSettings is undefined (e.g., disabled or loading query), even though the hook falls back to global settings. Treat both null and undefined as “no per-chat override.”
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/useChatSettings.ts, line 71:
<comment>`isUsingGlobalSettings` reports false when `chatSettings` is undefined (e.g., disabled or loading query), even though the hook falls back to global settings. Treat both `null` and `undefined` as “no per-chat override.”</comment>
<file context>
@@ -0,0 +1,84 @@
+
+ // Check if using global settings (no per-chat override)
+ isUsingGlobalSettings: {
+ chatMode: chatSettings?.chatMode === null,
+ model: chatSettings?.selectedModel === null,
+ },
</file context>
| updates.selectedModel = selectedModel; | ||
| } | ||
|
|
||
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); |
There was a problem hiding this comment.
P2: Guard against empty updates. If no fields are provided, return early (or throw a validation error) before calling update; otherwise this can execute an UPDATE with no SET columns and fail at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/chat_handlers.ts, line 207:
<comment>Guard against empty updates. If no fields are provided, return early (or throw a validation error) before calling update; otherwise this can execute an UPDATE with no SET columns and fail at runtime.</comment>
<file context>
@@ -172,5 +172,40 @@ export function registerChatHandlers() {
+ updates.selectedModel = selectedModel;
+ }
+
+ await db.update(chats).set(updates).where(eq(chats.id, chatId));
+ });
+
</file context>
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | |
| if (Object.keys(updates).length === 0) { | |
| return; | |
| } | |
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); |
There was a problem hiding this comment.
Code Review
This pull request successfully introduces per-chat settings for chat mode and the selected model, which is a great enhancement for user experience. The changes are well-structured across the database, backend, and frontend. The new useChatSettings hook is a clean abstraction for managing these settings with a proper fallback mechanism to global settings. The addition of an end-to-end test is excellent and ensures the core functionality is robust. I have one suggestion to improve the correctness of the new hook for future use.
| chatMode: chatSettings?.chatMode === null, | ||
| model: chatSettings?.selectedModel === null, |
There was a problem hiding this comment.
The logic for isUsingGlobalSettings is not entirely correct for all states. When chatId is null or when the per-chat settings are still loading, chatSettings is undefined. In this case, chatSettings?.chatMode is undefined, and undefined === null evaluates to false. This incorrectly implies that global settings are not being used, when in fact they are.
To make this property reliable for future use, you can use == null which checks for both null and undefined.
| chatMode: chatSettings?.chatMode === null, | |
| model: chatSettings?.selectedModel === null, | |
| chatMode: chatSettings?.chatMode == null, | |
| model: chatSettings?.selectedModel == null, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4658c55d7e
ℹ️ 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".
| const selectedMode = selectedChatId | ||
| ? effectiveChatMode | ||
| : settings?.selectedChatMode || "build"; |
There was a problem hiding this comment.
Use route chat id before applying per-chat settings
These selectors now switch to per-chat state whenever selectedChatId is non-null, but that atom is not cleared on every non-chat navigation (for example AppList.handleNewApp only calls navigate({ to: "/" })). In that case, HomeChatInput still renders these controls and changing mode/model on the home screen mutates the previously opened chat instead of updating global defaults, which is a data-scope regression introduced by the new selectedChatId gate.
Useful? React with 👍 / 👎.
| selectedChatMode: chat.chatMode ?? globalSettings.selectedChatMode, | ||
| // Per-chat model overrides global if set | ||
| selectedModel: chat.selectedModel ?? globalSettings.selectedModel, |
There was a problem hiding this comment.
Keep token counting consistent with per-chat overrides
Streaming now applies per-chat selectedChatMode/selectedModel, but chat:count-tokens still reads only global settings (src/ipc/handlers/token_count_handlers.ts:61-74), so chats with overrides get incorrect context-window estimates and warning behavior in the UI (useCountTokens). This divergence is introduced here by making generation settings chat-specific without updating the token-count path to use the same per-chat values.
Useful? React with 👍 / 👎.
🔍 Multi-Agent Code ReviewFound 7 issue(s) flagged by consensus of 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (2 items)
See inline comments for details. Generated by multi-agent consensus review (3 agents, consensus threshold: 2) |
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | ||
| const { chatId, chatMode, selectedModel } = params; | ||
|
|
||
| // Build the update object dynamically | ||
| const updates: Record<string, unknown> = {}; | ||
|
|
||
| if (chatMode !== undefined) { | ||
| updates.chatMode = chatMode; | ||
| } | ||
| if (selectedModel !== undefined) { | ||
| updates.selectedModel = selectedModel; | ||
| } | ||
|
|
||
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); |
There was a problem hiding this comment.
🟡 MEDIUM — updateChatSettings does not verify the chat exists before updating (3/3 agents flagged)
Unlike getChatSettings which throws "Chat not found", updateChatSettings will silently succeed with 0 rows affected if called with a non-existent chatId. This could mask bugs where the frontend tries to update settings for a deleted chat.
Additionally, if both chatMode and selectedModel are undefined, the updates object will be empty and an unnecessary db.update(chats).set({}) executes.
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | |
| const { chatId, chatMode, selectedModel } = params; | |
| // Build the update object dynamically | |
| const updates: Record<string, unknown> = {}; | |
| if (chatMode !== undefined) { | |
| updates.chatMode = chatMode; | |
| } | |
| if (selectedModel !== undefined) { | |
| updates.selectedModel = selectedModel; | |
| } | |
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | |
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | |
| const { chatId, chatMode, selectedModel } = params; | |
| // Build the update object dynamically | |
| const updates: Partial<typeof chats.$inferInsert> = {}; | |
| if (chatMode !== undefined) { | |
| updates.chatMode = chatMode; | |
| } | |
| if (selectedModel !== undefined) { | |
| updates.selectedModel = selectedModel; | |
| } | |
| if (Object.keys(updates).length === 0) { | |
| return; | |
| } | |
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | |
| }); |
| isUsingGlobalSettings: { | ||
| chatMode: chatSettings?.chatMode === null, | ||
| model: chatSettings?.selectedModel === null, | ||
| }, |
There was a problem hiding this comment.
🟡 MEDIUM — isUsingGlobalSettings returns incorrect values during loading state (3/3 agents flagged)
When chatSettings is undefined (query loading or chatId is null), chatSettings?.chatMode === null evaluates to false (undefined === null is false). This incorrectly reports the user is NOT using global settings when the data hasn't loaded yet.
This field doesn't appear to be consumed anywhere in the codebase currently. Consider removing it, or if needed, fix the logic to handle the loading state:
| isUsingGlobalSettings: { | |
| chatMode: chatSettings?.chatMode === null, | |
| model: chatSettings?.selectedModel === null, | |
| }, | |
| isUsingGlobalSettings: { | |
| chatMode: !chatSettings || chatSettings.chatMode === null, | |
| model: !chatSettings || chatSettings.selectedModel === null, | |
| }, |
| selectedModel: text("selected_model", { mode: "json" }).$type<{ | ||
| name: string; | ||
| provider: string; | ||
| customModelId?: number; | ||
| } | null>(), |
There was a problem hiding this comment.
🟡 MEDIUM — Inline type duplicates LargeLanguageModel definition (3/3 agents flagged)
The structural type { name: string; provider: string; customModelId?: number } duplicates the LargeLanguageModel type from src/lib/schemas.ts. If LargeLanguageModel gains new fields in the future, this type won't be updated, causing a silent type mismatch between the DB layer and the rest of the app.
Consider importing and referencing the canonical type:
import type { LargeLanguageModel } from "@/lib/schemas";
// ...
selectedModel: text("selected_model", { mode: "json" }).$type<LargeLanguageModel | null>(),| if (!settings) { | ||
| return null; | ||
| } | ||
| const selectedModel = settings?.selectedModel; | ||
| // Use per-chat model if available, otherwise fall back to global settings |
There was a problem hiding this comment.
🟡 MEDIUM — getModelDisplayName (defined at line 87) has a fragile forward reference to selectedModel (2/3 agents flagged)
The getModelDisplayName function references selectedModel which isn't assigned until here. While it works because the function is only called after this assignment, a future refactor that calls getModelDisplayName() before this line would cause a ReferenceError.
Consider moving this selectedModel declaration above the getModelDisplayName function definition, or refactoring getModelDisplayName to accept selectedModel as a parameter.
4658c55 to
ac87ef3
Compare
|
@BugBot run |
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | ||
| const { chatId, chatMode, selectedModel } = params; | ||
|
|
||
| // Build the update object dynamically | ||
| const updates: Record<string, unknown> = {}; | ||
|
|
||
| if (chatMode !== undefined) { | ||
| updates.chatMode = chatMode; | ||
| } | ||
| if (selectedModel !== undefined) { | ||
| updates.selectedModel = selectedModel; | ||
| } | ||
|
|
||
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | ||
| }); |
There was a problem hiding this comment.
Missing chat existence validation before update. The getChatSettings handler verifies the chat exists and throws an error if not found, but updateChatSettings silently succeeds even for non-existent chat IDs. This inconsistency could mask bugs.
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | |
| const { chatId, chatMode, selectedModel } = params; | |
| // Build the update object dynamically | |
| const updates: Record<string, unknown> = {}; | |
| if (chatMode !== undefined) { | |
| updates.chatMode = chatMode; | |
| } | |
| if (selectedModel !== undefined) { | |
| updates.selectedModel = selectedModel; | |
| } | |
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | |
| }); | |
| createTypedHandler(chatContracts.updateChatSettings, async (_, params) => { | |
| const { chatId, chatMode, selectedModel } = params; | |
| // Verify chat exists before updating | |
| const chat = await db.query.chats.findFirst({ | |
| where: eq(chats.id, chatId), | |
| columns: { id: true }, | |
| }); | |
| if (!chat) { | |
| throw new Error("Chat not found"); | |
| } | |
| // Build the update object dynamically | |
| const updates: Record<string, unknown> = {}; | |
| if (chatMode !== undefined) { | |
| updates.chatMode = chatMode; | |
| } | |
| if (selectedModel !== undefined) { | |
| updates.selectedModel = selectedModel; | |
| } | |
| await db.update(chats).set(updates).where(eq(chats.id, chatId)); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/handlers/chat_handlers.ts
Line: 199:213
Comment:
Missing chat existence validation before update. The `getChatSettings` handler verifies the chat exists and throws an error if not found, but `updateChatSettings` silently succeeds even for non-existent chat IDs. This inconsistency could mask bugs.
```suggestion
createTypedHandler(chatContracts.updateChatSettings, async (_, params) => {
const { chatId, chatMode, selectedModel } = params;
// Verify chat exists before updating
const chat = await db.query.chats.findFirst({
where: eq(chats.id, chatId),
columns: { id: true },
});
if (!chat) {
throw new Error("Chat not found");
}
// Build the update object dynamically
const updates: Record<string, unknown> = {};
if (chatMode !== undefined) {
updates.chatMode = chatMode;
}
if (selectedModel !== undefined) {
updates.selectedModel = selectedModel;
}
await db.update(chats).set(updates).where(eq(chats.id, chatId));
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Devin Review found 3 new potential issues.
🐛 3 issues in files not directly in the diff
🐛 Keyboard shortcut (Cmd+.) always toggles global chat mode, ignoring per-chat settings (src/hooks/useChatModeToggle.ts:24-33)
The useChatModeToggle hook always calls updateSettings({ selectedChatMode: newMode }) (line 33), which updates the global setting. When a chat is selected with a per-chat mode override, pressing Cmd+. (or Ctrl+.) changes the global mode instead of the per-chat mode. This creates an inconsistent state: the ChatModeSelector reads the per-chat mode via useChatSettings, but the keyboard shortcut modifies the global mode, so the UI won't reflect the shortcut's change.
Root Cause and Impact
The ChatModeSelector (src/components/ChatModeSelector.tsx:61-64) correctly routes mode changes to per-chat settings when selectedChatId is set, but the useChatModeToggle hook was not updated for the per-chat settings feature. It reads settings.selectedChatMode (global) and writes back to global settings at src/hooks/useChatModeToggle.ts:33:
updateSettings({ selectedChatMode: newMode });Impact: When a user uses the keyboard shortcut to toggle chat mode while a chat is selected, the wrong settings are modified. The global mode changes but the per-chat mode (which the UI reads from) stays the same, making the shortcut appear broken. Worse, it silently modifies global settings as a side effect.
🐛 ChatInput reads global selectedChatMode for UI logic instead of per-chat effective mode (src/components/chat/ChatInput.tsx:176)
After this PR, ChatModeSelector and the streaming backend correctly use per-chat mode overrides, but ChatInput still reads settings?.selectedChatMode (global) for multiple UI-behavior decisions. When a chat has a per-chat mode override that differs from the global mode, ChatInput's UI logic will be wrong.
Affected Code Paths in ChatInput
-
Send button disabling (
src/components/chat/ChatInput.tsx:176):settings?.selectedChatMode !== "local-agent"— if per-chat mode islocal-agentbut global isbuild, the send button will be incorrectly disabled when there's an unapproved proposal. -
Plan mode fresh chat detection (
src/components/chat/ChatInput.tsx:199-211):prevModeReftrackssettings?.selectedChatMode(global). If the user switches a specific chat to plan mode viaChatModeSelector, the global mode isn't changed, so this transition detection never fires. -
Plan mode fresh chat creation (
src/components/chat/ChatInput.tsx:257):settings?.selectedChatMode === "plan"checks global, so switching a per-chat mode to plan won't trigger the fresh chat flow. -
Proposal actions visibility (
src/components/chat/ChatInput.tsx:487-488):settings.selectedChatMode !== "ask" && settings.selectedChatMode !== "local-agent"— if per-chat mode isaskbut global isbuild, proposal actions will incorrectly appear.
Impact: The frontend UI behavior (button states, proposal display, plan mode handling) won't match the actual per-chat mode, causing a confusing and broken user experience when per-chat overrides differ from global settings.
⚠️ Creating new chat modifies global selectedChatMode affecting other chats without per-chat overrides (src/components/ChatList.tsx:104)
In ChatList.tsx:104, when creating a new chat, the code calls updateSettings({ selectedChatMode: effectiveDefaultMode }) which modifies the global selectedChatMode setting rather than setting a per-chat mode for the new chat.
Root Cause
With the new per-chat settings feature, the global selectedChatMode serves as a fallback for chats that don't have an explicit per-chat mode set. When ChatList.tsx modifies the global setting on every new chat creation, it can inadvertently change the displayed mode for all other existing chats that rely on the global fallback.
Example scenario:
- User sets global default to "build"
- User creates Chat #1, explicitly sets it to "ask" (stored as per-chat setting)
- User creates Chat #2 - code resets global to "local-agent" (from
getEffectiveDefaultChatMode) - Now Chat #1 still shows "ask" (has per-chat override), but any chat without per-chat settings will show "local-agent" instead of the user's intended "build" default
Impact: The global chat mode setting gets overwritten every time a new chat is created, causing unexpected mode changes for chats relying on the global fallback.
View 8 additional findings in Devin Review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac87ef3864
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (selectedChatId) { | ||
| updatePerChatMode(newMode); | ||
| } else { |
There was a problem hiding this comment.
Propagate per-chat mode to all chat-mode consumers
This change writes mode updates to per-chat state (updatePerChatMode) when a chat is selected, but other mode-dependent paths still read/write only global settings (useChatModeToggle updates settings.selectedChatMode, and ChatInput gates behavior off settings.selectedChatMode in its send-disable and plan-transition logic). In a chat whose override differs from global (for example chat override = plan, global = build), Cmd+. toggling and several input behaviors operate on stale global mode, so the UI no longer matches the mode actually used by streaming.
Useful? React with 👍 / 👎.
| // Use per-chat aware settings (already defined earlier in this function) | ||
| if ( | ||
| settings.autoApproveChanges && | ||
| settings.selectedChatMode !== "ask" |
There was a problem hiding this comment.
Re-read approval settings before auto-applying edits
The auto-apply branch now relies on the settings snapshot created at stream start, because the previous end-of-stream readSettings() refresh was removed here. If a user turns off autoApproveChanges while a long response is streaming, this branch can still auto-apply edits using stale settings, which is a regression from the prior behavior that reloaded settings immediately before processFullResponseActions.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review Summary✅ No new issues found by consensus review. (7 issue(s) already commented on in prior reviews) Prior multi-agent review and other reviewers have already identified and commented on the key issues in this PR:
Generated by Dyadbot code review (3 agents, consensus threshold: 2) |
🎭 Playwright Test Results❌ Some tests failed
Summary: 690 passed, 7 failed, 12 flaky, 224 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/free_agent_quota.spec.ts \
e2e-tests/mention_app.spec.ts \
e2e-tests/plan_mode.spec.ts \
e2e-tests/smart_context_deep.spec.ts
|
Closes: #1844
Summary by cubic
Adds per‑chat settings for chat mode and selected model so each chat can have its own configuration, with global settings used as fallback. Streaming and UI now respect these per‑chat overrides.
New Features
Migration
Written for commit 4658c55. Summary will update on new commits.