PR Summary: Chat Mode Persistence & Code Review Fixes#3142
PR Summary: Chat Mode Persistence & Code Review Fixes#3142nourzakhama2003 wants to merge 21 commits intodyad-sh:mainfrom
Conversation
|
@BugBot run |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Three HIGH-severity issues involve race conditions / rollback paths in the new per-chat mode persistence, and a fourth puts the user back into the broken state they were trying to escape. Multiple MEDIUM issues compound the risk. Issues Summary
🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nourzakhama2003
force-pushed
the
fix/chat-mode-code-review-followup
branch
from
87624c9 to
3d0a366
Compare
|
@BugBot run |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Most of the high-severity concerns raised in earlier review rounds (the race conditions around optimistic rollback, quota-banner switch ordering, legacy-migration imprint, and the New Issues Summary
🚫 Dropped / Not re-flagged (already covered by earlier comments)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Findings below are new — issues already flagged by other reviewers (gemini-code-assist, dyad-assistant, cubic-dev-ai, chatgpt-codex, devin) were deduplicated. 🔴 HIGH
🟡 MEDIUM
🟢 Low Priority Notes
🚫 Dropped / Duplicates
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
bot
added
the
needs-human:review-issue
| (effectiveChatMode !== "local-agent" && effectiveChatMode !== undefined) || | ||
| (settings?.selectedChatMode !== "local-agent" && | ||
| effectiveChatMode === undefined && | ||
| lastMessage?.role === "assistant" && | ||
| !lastMessage.approvalState && | ||
| !!proposal && | ||
| proposal.type === "code-proposal" && | ||
| messageId === lastMessage.id); |
There was a problem hiding this comment.
🔴 HIGH | correctness / regression
disableSendButton unconditionally disables send for all non-agent modes
The new first OR clause (effectiveChatMode !== "local-agent" && effectiveChatMode !== undefined) forces disableSendButton = true whenever effectiveChatMode is anything other than "local-agent". Because ChatPanel.tsx:59-60 always resolves effectiveChatMode = currentChat?.chatMode ?? settings?.selectedChatMode ?? "build" (never undefined), this first clause is true for every build / ask / plan chat, which blocks the send button entirely. The second OR clause (with the proposal checks) is only reachable when effectiveChatMode === undefined — which never happens from ChatPanel.
The original pre-PR logic only blocked sends when there was an unapproved assistant code proposal. This PR's attempt to fix an earlier review comment introduced a full regression: users cannot send any message in Build, Ask, or Plan mode.
💡 Suggestion: Replace with a single proposal-gated check that uses the effective mode:
const resolvedMode = effectiveChatMode ?? settings?.selectedChatMode;
const disableSendButton =
resolvedMode !== "local-agent" &&
lastMessage?.role === "assistant" &&
!lastMessage.approvalState &&
!!proposal &&
proposal.type === "code-proposal" &&
messageId === lastMessage.id;| <span className="text-sm">{messages.length} Queued</span> | ||
| <span className="text-xs text-muted-foreground">- {statusText}</span> | ||
| {!isPaused && ( | ||
| <span className="text-xs text-muted-foreground"> | ||
| - {statusText} | ||
| </span> | ||
| )} | ||
| {isPaused && ( | ||
| <span className="text-xs px-2 py-0.5 rounded bg-yellow-500/20 text-yellow-700 dark:text-yellow-400 font-medium"> | ||
| Paused | ||
| </span> | ||
| )} | ||
| </div> | ||
| <div className="flex items-center gap-2 flex-shrink-0 ml-3"> | ||
| {isExpanded ? ( | ||
| <ChevronUp className="w-4 h-4 text-muted-foreground" /> | ||
| ) : ( | ||
| <ChevronDown className="w-4 h-4 text-muted-foreground" /> | ||
| )} | ||
| <button | ||
| type="button" | ||
| onClick={isPaused ? onResumeQueue : onPauseQueue} | ||
| aria-label={isPaused ? "Resume queue" : "Pause queue"} | ||
| title={isPaused ? "Resume queue" : "Pause queue"} | ||
| className={cn( |
There was a problem hiding this comment.
🟡 MEDIUM | i18n
Queue header introduces several hard-coded English strings
The new queue/pause UI adds user-visible English strings that are not translated: statusText variants ("will send after a successful response", "will send after current response", "ready to send") at L122-126; the "{n} Queued" label at L152; the "Paused" badge at L160; and the button aria-label / title values "Pause queue" / "Resume queue" at L168-169 and "Collapse" / "Expand" at L149 and L183. Non-English users (pt-BR, zh-CN) will see mixed-language UI every time the queue renders, and screen-reader labels remain English-only.
💡 Suggestion: Move these strings into chat.json under a chatMode.queue.* (or similar) namespace and translate into pt-BR/zh-CN, matching the pattern used elsewhere in this file/PR.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Validated against 279 existing inline comments and 117 issue comments from prior review rounds. Issues Summary
The HIGH issue is a blocker: the refactor from 🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives / Duplicates (10 items)
Generated by Dyadbot multi-agent code review |
…ys, and medium priority fixes
…ion, and UX fixes - Wait for quota readiness before picking conflict chat mode to prevent quota errors - Set lastRestoredChatIdRef for legacy chats to prevent repeated IPC calls - Pass isProEnabled to getChatModeLabelKey so non-Pro users see correct label - Remove dead persistTimeoutRef and cleanup effect from ChatModeSelector
…users default to build
- Revert all E2E snapshot changes to main branch state - Remove resolve-git-conflict.ts fixture file This commit removes unrelated changes from the chat mode persistence PR, keeping only the core functionality.
- Revert getEffectiveDefaultChatMode to allow non-pro users with quota to use local-agent mode (as requested by maintainer) - Remove chat mode selection complexity from useResolveMergeConflictsWithAI (as requested by maintainer) - E2E snapshot changes already reverted to main in previous commit
🎭 Playwright Test Results❌ Some tests failed
Summary: 613 passed, 134 failed, 11 flaky, 268 skipped Failed Tests🍎 macOSShow all 94 failures
🪟 WindowsShow all 40 failures
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/annotator.spec.ts \
e2e-tests/astro.spec.ts \
e2e-tests/attach_image.spec.ts \
e2e-tests/cancelled_message.spec.ts \
e2e-tests/chat_history.spec.ts \
e2e-tests/chat_image_generation.spec.ts \
e2e-tests/chat_input.spec.ts \
e2e-tests/chat_mode.spec.ts \
e2e-tests/context_compaction.spec.ts \
e2e-tests/context_manage.spec.ts \
e2e-tests/context_window.spec.ts \
e2e-tests/dyad_tags_parsing.spec.ts \
e2e-tests/engine.spec.ts \
e2e-tests/fix_error.spec.ts \
e2e-tests/free_agent_quota.spec.ts \
e2e-tests/github.spec.ts \
e2e-tests/import.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/local_agent_ask.spec.ts \
e2e-tests/local_agent_basic.spec.ts \
e2e-tests/local_agent_connection_retry.spec.ts \
e2e-tests/local_agent_consent.spec.ts \
e2e-tests/local_agent_generate_image.spec.ts \
e2e-tests/local_agent_grep.spec.ts \
e2e-tests/local_agent_list_files.spec.ts \
e2e-tests/local_agent_persistent_todos.spec.ts \
e2e-tests/local_agent_run_type_checks.spec.ts \
e2e-tests/local_agent_search_replace.spec.ts \
e2e-tests/local_agent_step_limit.spec.ts \
e2e-tests/local_agent_summarize.spec.ts \
e2e-tests/local_agent_web_fetch.spec.ts \
e2e-tests/mcp.spec.ts \
e2e-tests/mention_files.spec.ts \
e2e-tests/partial_response.spec.ts \
e2e-tests/pause_queue.spec.ts \
e2e-tests/plan_mode.spec.ts \
e2e-tests/problems.spec.ts \
e2e-tests/queued_message.spec.ts \
e2e-tests/rename_edit.spec.ts \
e2e-tests/security_review.spec.ts \
e2e-tests/select_component.spec.ts \
e2e-tests/smart_context_deep.spec.ts \
e2e-tests/socket_firewall.spec.ts \
e2e-tests/supabase_client.spec.ts \
e2e-tests/supabase_migrations.spec.ts \
e2e-tests/telemetry.spec.ts \
e2e-tests/thinking_budget.spec.ts \
e2e-tests/turbo_edits_v2.spec.ts \
e2e-tests/undo.spec.ts \
e2e-tests/version_integrity.spec.ts \
e2e-tests/visual_editing.spec.ts
|
|
@BugBot run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd97e9ed55
ℹ️ 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 (!appId) { | ||
| console.warn( | ||
| `Skipping chat mode persist for chat ${chatId}: appId not available`, | ||
| ); | ||
| // Still apply the resolved mode to settings even without DB persistence | ||
| shouldUpdateSelectedChatMode = true; |
There was a problem hiding this comment.
When a disallowed mode is restored on a deep-linked chat (appId is null), this branch skips persistence and sets shouldUpdateSelectedChatMode, but never clears isRestoringMode. If the banner was shown (e.g., cache miss + slow getChat), the "Restoring chat mode..." indicator can remain visible indefinitely for that chat. Please clear restoring state in the !appId fallback path as well.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE — a handful of net-new issues worth addressing Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Files were shuffled per reviewer to reduce ordering bias. This PR has 284+ prior inline review comments across many rounds. Agents were instructed to deduplicate aggressively against existing feedback. The issues below are net-new — not previously flagged — and each was validated against the current source. New Issues
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives / Duplicates (5 items)
Generated by Dyadbot multi-agent code review |
| const newChatId = await ipc.chat.createChat(appId); | ||
| const newChatId = await ipc.chat.createChat({ | ||
| appId, | ||
| initialChatMode: undefined, |
There was a problem hiding this comment.
🔴 HIGH | interaction
Merge-conflict resolution silently breaks in Ask/Plan mode
createChat is called with initialChatMode: undefined, so the newly-created resolution chat inherits the user's global selectedChatMode (e.g. ask or plan). Further down, chatStream.start is called without a chatMode override, so the stream also runs in whatever mode the user has set globally. In Ask mode the model cannot edit files, and in Plan mode it only proposes changes — neither can actually resolve the merge conflict.
Users whose default mode is Ask or Plan will click the resolve button and get an apparently-successful response that doesn't touch the files on disk, with no indication of why.
💡 Suggestion: Pass initialChatMode: "build" (or "local-agent" where appropriate) to createChat, and pass a matching chatMode to chatStream.start, so the resolution chat always runs in a mode that can edit files regardless of the user's global preference.
| if (!freeAgentQuotaAvailable) return false; | ||
| // For non-Pro users, local-agent is only allowed when OpenAI or Anthropic | ||
| // is configured; otherwise basic quota should not enable the agent mode. | ||
| return isOpenAIOrAnthropicSetup(settings, envVars); |
There was a problem hiding this comment.
🟡 MEDIUM | dead-code
isBasicAgentMode helper (just below at line 504) is now dead code
This PR removes every import of isBasicAgentMode — see the -import { isBasicAgentMode } lines in src/components/ChatPanel.tsx, src/ipc/handlers/chat_stream_handlers.ts, and the other handler file. A repo-wide search turns up no remaining call sites: every other match is a locally-scoped const isBasicAgentMode = ... variable, not a reference to this exported function.
The exported helper in src/lib/schemas.ts:504-508 is now unused infrastructure and should be removed.
💡 Suggestion: Delete the exported isBasicAgentMode function. If it's being kept intentionally for future use, add a comment explaining why.
| }), | ||
| { id: `restore-timeout-${chatId}` }, | ||
| ); | ||
| setIsRestoringMode(false); |
There was a problem hiding this comment.
🟡 MEDIUM | logic
Timeout branch doesn't mark the chat as restored, allowing duplicate toasts and reruns
In the success branches of this effect, lastRestoredChatIdRef.current = chatId is set so the restore logic won't re-run for the same chat. The timeout/error branch here calls setIsRestoringMode(false) and shows the restoreModeTimedOut toast, but never updates lastRestoredChatIdRef.current.
If the effect re-runs for the same chatId (e.g. a late settings update, a stream count bump, or a dependency change), the restore will fire again and the user sees the warning toast a second time.
💡 Suggestion: Set lastRestoredChatIdRef.current = chatId in the timeout branch too, so the restore is marked as terminally attempted.
| showError( | ||
| t("chatMode.planAgentUnavailable", { | ||
| defaultValue: | ||
| "Agent mode is unavailable for plan implementation. Configure an OpenAI or Anthropic provider, or upgrade to Pro.", |
There was a problem hiding this comment.
🟡 MEDIUM | i18n
defaultValue contradicts the actual locale copy
The inline defaultValue here is "Agent mode is unavailable for plan implementation. Configure an OpenAI or Anthropic provider, or upgrade to Pro.", but every locale file (en, pt-BR, zh-CN) defines chatMode.planAgentUnavailable with different remediation copy ("...Please enable an eligible mode and try again.") that makes no mention of providers or Pro.
In practice users will see the translated string, but if the key is ever missing or renamed they'll get a fundamentally different message. The two copies will also silently drift further apart over time.
💡 Suggestion: Either drop the defaultValue entirely (let t() fall back to the key) or align it with the locale copy so both describe the same remediation.
| @@ -496,6 +504,7 @@ export function ChatInput({ chatId }: { chatId?: number }) { | |||
| await streamMessage({ | |||
| prompt: promptWithImages, | |||
There was a problem hiding this comment.
🟡 MEDIUM | i18n
Hardcoded English string a few lines above this hunk (line 502)
Just above this added line — at src/components/chat/ChatInput.tsx:502 — is showInfo("We've switched you to a new chat for a clean context"), still hardcoded in English. The switchedToNewChatCleanContext key is already defined in all three locale files (en, pt-BR, zh-CN) specifically for this message, so non-English users see English here while the rest of the UI is translated.
💡 Suggestion: Replace with showInfo(t("switchedToNewChatCleanContext")) (or the appropriate namespaced key).
| if (settings?.selectedChatMode !== writableMode) { | ||
| const modeLabel = | ||
| writableMode === "local-agent" | ||
| ? t("chatMode.agent", { defaultValue: "Agent" }) |
There was a problem hiding this comment.
🟡 MEDIUM | consistency
Warning toast shows "Agent" even when the selector shows "Basic Agent"
This uses t("chatMode.agent", { defaultValue: "Agent" }) unconditionally, but non-Pro users see the same mode labeled as "Basic Agent" in ChatModeSelector (which branches on isProEnabled). A non-Pro user who triggers this warning is told the review "runs in Agent mode" but the selector only offers "Basic Agent" — two names for the same thing, one more place users have to reconcile.
💡 Suggestion: Branch on isDyadProEnabled(settings) (the same condition the selector uses) and pick chatMode.basicAgent vs chatMode.agent accordingly.
| }); | ||
|
|
||
| updateSettingsRef | ||
| .current({ selectedChatMode: resolvedMode.mode }) |
There was a problem hiding this comment.
🟡 MEDIUM | consistency
Fallback mode silently overrides the user's selection with no toast
When resolvedMode.usedFallback === true, this block quietly writes the fallback mode via updateSettings({ selectedChatMode: resolvedMode.mode }) and moves on. The parallel code path in useRestoreChatMode handles the same scenario by showing a chatMode.modeUnavailableFallback toast so the user understands why their chosen mode was overridden.
As-is, a user switching between chat tabs (or jumping to a chat whose persisted mode is no longer allowed) can have their selected mode changed with no visible feedback — a UX regression from the symmetric hook.
💡 Suggestion: Check resolvedMode.usedFallback and show the same modeUnavailableFallback toast here, so the two hooks behave consistently.
| )} | ||
| > | ||
| <div className="flex items-center gap-2 mb-0.5"> | ||
| <span className="text-foreground/80">{icon}</span> |
There was a problem hiding this comment.
🟡 MEDIUM | ux
Mode icons lose their color coding inside the dropdown options
The trigger button colors selectedMode icons by mode (e.g. ask → purple-500, plan → blue-500) — that's how users learn the mode↔color association at a glance. But ModeOption wraps the icon in <span className="text-foreground/80">{icon}</span>, which overrides those per-mode colors with a neutral foreground tone.
Result: the dropdown strips exactly the visual affordance that the trigger relies on, so scanning the list for the mode you want is noticeably harder than it needs to be.
💡 Suggestion: Drop the text-foreground/80 wrapper (or only apply it when the option is disabled), so the per-mode icon colors carry through from trigger to dropdown.
🎭 Playwright Test Results❌ Some tests failed
Summary: 602 passed, 140 failed, 10 flaky, 268 skipped Failed Tests🍎 macOSShow all 97 failures
🪟 WindowsShow all 43 failures
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/annotator.spec.ts \
e2e-tests/astro.spec.ts \
e2e-tests/attach_image.spec.ts \
e2e-tests/cancelled_message.spec.ts \
e2e-tests/chat_history.spec.ts \
e2e-tests/chat_image_generation.spec.ts \
e2e-tests/chat_input.spec.ts \
e2e-tests/chat_mode.spec.ts \
e2e-tests/context_compaction.spec.ts \
e2e-tests/context_manage.spec.ts \
e2e-tests/context_window.spec.ts \
e2e-tests/dyad_tags_parsing.spec.ts \
e2e-tests/engine.spec.ts \
e2e-tests/free_agent_quota.spec.ts \
e2e-tests/github.spec.ts \
e2e-tests/import.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/local_agent_ask.spec.ts \
e2e-tests/local_agent_basic.spec.ts \
e2e-tests/local_agent_code_search.spec.ts \
e2e-tests/local_agent_connection_retry.spec.ts \
e2e-tests/local_agent_consent.spec.ts \
e2e-tests/local_agent_file_upload.spec.ts \
e2e-tests/local_agent_generate_image.spec.ts \
e2e-tests/local_agent_list_files.spec.ts \
e2e-tests/local_agent_persistent_todos.spec.ts \
e2e-tests/local_agent_read_logs.spec.ts \
e2e-tests/local_agent_run_type_checks.spec.ts \
e2e-tests/local_agent_step_limit.spec.ts \
e2e-tests/local_agent_summarize.spec.ts \
e2e-tests/local_agent_todo_followup.spec.ts \
e2e-tests/local_agent_web_fetch.spec.ts \
e2e-tests/mcp.spec.ts \
e2e-tests/mention_files.spec.ts \
e2e-tests/partial_response.spec.ts \
e2e-tests/pause_queue.spec.ts \
e2e-tests/plan_mode.spec.ts \
e2e-tests/problems.spec.ts \
e2e-tests/queued_message.spec.ts \
e2e-tests/rename_edit.spec.ts \
e2e-tests/security_review.spec.ts \
e2e-tests/select_component.spec.ts \
e2e-tests/smart_context_deep.spec.ts \
e2e-tests/socket_firewall.spec.ts \
e2e-tests/supabase_client.spec.ts \
e2e-tests/supabase_migrations.spec.ts \
e2e-tests/telemetry.spec.ts \
e2e-tests/thinking_budget.spec.ts \
e2e-tests/turbo_edits_v2.spec.ts \
e2e-tests/undo.spec.ts \
e2e-tests/version_integrity.spec.ts \
e2e-tests/visual_editing.spec.ts
|
2 participants
Closes #2926
Summary
This PR introduces robust per-chat mode persistence, addressing all code review feedback (Dyadbot + Devin) and modernizing the chat mode architecture for flexibility, reliability, and maintainability.
Key Improvements
Each chat now stores its own chatMode in the database (optional for backward compatibility).
Existing chats with chatMode: null default to the global user setting.
UI updates both the immediate session (settings.selectedChatMode) and persists to DB (ipc.chat.updateChatMode()).
On chat switch, the app restores the mode from the chat’s DB value if set, otherwise falls back to the global setting.
Stream path uses a non-blocking formula:
effectiveStreamMode = chat.chatMode ?? settings.selectedChatMode
All updates are coordinated to prevent race conditions between DB and settings.
“Build + Send” and keyboard shortcut operations are now optimistic: UI updates instantly, with rollback if DB persist fails.
Re-entrancy guard prevents rapid keypresses from queuing multiple async operations.
ChatPanel restore effect uses a cleanup flag pattern to prevent stale async results from overwriting state.
useSelectChat fallback now waits for DB persist before showing notifications, preventing repeated toasts.
ChatModeSelector’s complex ternary logic is now a readable helper function, improving maintainability.
Users can assign and persist different modes per chat, with seamless fallback and clear feedback if a mode becomes unavailable.
All eligibility checks and fallbacks are coordinated to ensure a smooth, predictable experience.
Testing
E2E:
npm run e2e -- chat_mode_persistence.spec.ts
All core scenarios and regressions are covered, including mode switching, persistence, eligibility fallback, and rapid user actions.