Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded manual-title tracking and requestedCwd propagation to agent chat sessions; introduced permission-mode re-synchronization across prewarm/resume/turns; adjusted PTY title-generation timing/thresholds; refined git divergence recommendation; added session-rename UI and multiple tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
arul28
force-pushed
the
ade/chat-title-summaries-30b9f371
branch
from
8142a02 to
0c3ac86
Compare
arul28
force-pushed
the
ade/chat-title-summaries-30b9f371
branch
from
0c3ac86 to
ebdf392
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts (1)
109-115: Consider deriving modes from the type to avoid drift.The hardcoded
modesarray duplicates theAgentChatPermissionModevalues. If new modes are added to the type, this test will still pass but won't verify the new mode. Since each mode is already explicitly tested above, this is low-risk, but using a type-driven approach could future-proof the coverage check.💡 Optional: Extract modes to a shared constant
If a
PERMISSION_MODESconstant is defined alongside the type, both the source and tests can reference it:// In shared/types/chat.ts export const PERMISSION_MODES = ["default", "plan", "edit", "full-auto", "config-toml"] as const; export type AgentChatPermissionMode = (typeof PERMISSION_MODES)[number];Then in the test:
import { PERMISSION_MODES } from "../../../shared/types"; // ... for (const mode of PERMISSION_MODES) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts` around lines 109 - 115, The test hardcodes the modes array duplicating the AgentChatPermissionMode values which can drift; change the loop to derive modes from a single source of truth by importing and iterating over a PERMISSION_MODES constant (or the equivalent exported array) instead of the hardcoded list so new modes are automatically covered; update the test that calls buildTrackedCliStartupCommand({ provider: "claude" | "codex", permissionMode: mode }) to iterate PERMISSION_MODES and keep assertions the same, referencing AgentChatPermissionMode and buildTrackedCliStartupCommand to locate the test and the type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2846-2849: The current guard around starting auto-title generation
(using resolveChatConfig(), managed.manuallyNamed, managed.autoTitleInFlight)
prevents new requests but does not stop an already in-flight generation from
overwriting a user rename; after the async model call that ultimately calls
setManagedSessionTitle(...), re-check managed.manuallyNamed (and/or
managed.autoTitleInFlight token/version) before applying the returned title and
only call setManagedSessionTitle if the session is still not manually named and
the in-flight token/version matches, or alternatively attach/compare a unique
request id to invalidate older responses so an earlier response cannot clobber a
later manual rename.
- Around line 569-570: The manuallyNamed boolean isn't persisted so manual-title
state is lost on rehydrate; add a manuallyNamed property to PersistedChatState,
include it in the serialization/save path used by agentChatService (where chat
state is written), and restore it in readPersistedState() (or, if preferred,
infer manuallyNamed=true when the persisted title differs from the generated
default title) so a user-renamed chat remains protected from auto-titling after
app restart; update any initialization in agentChatService (references:
manuallyNamed, PersistedChatState, readPersistedState) to use the restored
value.
- Around line 8130-8137: The re-sync logic (when managed.runtime?.kind ===
"unified") currently uses resolveSessionUnifiedPermissionMode which prefers
stored session fields and so ignores config drift; change it to derive
permissionMode from the current project/chat config first (via resolveChatConfig
and a resolver that prefers config values) and only fall back to session-stored
permissions if config is unset — e.g., replace or augment
resolveSessionUnifiedPermissionMode call with a resolver that takes
resolveChatConfig() as the primary source or pass an option to ignore session
values; update the same pattern at the other locations that call
resolveSessionUnifiedPermissionMode (the blocks referenced at 8158-8170,
8526-8530, 8553-8557, 8563-8567) so runtime.permissionMode reflects current
config changes rather than stale session fields.
- Around line 9000-9003: The rename flow loses the manuallyNamed flag because
the payload parser in syncRemoteCommandService.ts drops it before calling
updateSession; updateSession and the session handler expect sessionId, title,
manuallyNamed, modelId, so modify the parser in the rename handling path (around
the code that parses the rename payload at the spots referenced ~1032-1033 and
the similar block ~9210-9223) to preserve and forward the manuallyNamed property
instead of omitting it; ensure the parsed object includes manuallyNamed and that
the callsite passes it through to updateSession (and any session update
handlers) so managed.manuallyNamed gets set and user renames aren’t overwritten
by auto-title logic.
In `@apps/desktop/src/main/services/git/gitOperationsService.ts`:
- Around line 611-618: The current logic runs
runGit(["merge-base","--is-ancestor",upstreamRef,"HEAD"]) while diverged is
true, which always returns non-zero and forces "force_push_lease"; change it so
that when diverged === true you default recommendedAction = "pull" (safe
default) instead of running merge-base, and only run the merge-base check when
not diverged (or when you have evidence the remote is strictly behind local).
Also treat git errors (mergeBaseRes.exitCode >= 128) as failures and fall back
to "pull" rather than treating them as "not ancestor"; keep the original mapping
mergeBaseRes.exitCode === 0 => "push", non-zero but <128 => "force_push_lease".
Locate symbols: diverged, runGit, mergeBaseRes, upstreamRef, lane.worktreePath,
and recommendedAction.
In `@apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts`:
- Around line 298-310: The test is asserting on the wrong mock instance; create
the mock laneService once, pass that same instance into
resolveLaneLaunchContext, and then assert on its getLaneBaseAndBranch call to
verify trimming. Concretely: call const laneService =
makeLaneService("/projects/my-lane") before resolveLaneLaunchContext, pass
laneService into resolveLaneLaunchContext, and replace the expect to check
laneService.getLaneBaseAndBranch was called (optionally with "lane-1") instead
of constructing a new makeLaneService for the assertion.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 411-412: The auto-title update unconditionally calls
sessionService.updateMeta({ sessionId, title/ finalTitle }) and can overwrite a
user rename; change the logic in ptyService where finalTitle (and the other
occurrence) is applied so you first read the current session meta (via
sessionService.getMeta or equivalent) and only call
sessionService.updateMeta(...) when the existing meta does not indicate a manual
rename (e.g., title is empty or an isAutoTitle flag is set); in short, guard the
calls to sessionService.updateMeta({ sessionId, title }) by checking the
session's current meta/manual-name flag and skip the update if the session has
been manually named.
- Around line 381-419: The title-refresh logic (using
getSessionIntelligence()?.titles?.refreshOnComplete and
aiIntegrationService.summarizeTerminal -> sessionService.updateMeta) is
currently placed after the summaries-enabled early return and after the summary
await, so it runs only when summaries are enabled and the summary call succeeds;
move this entire refresh-on-complete block so it executes independently: check
isTitleGenerationEnabled() and compute titlePrompt/resolveTitleModelId and call
aiIntegrationService.summarizeTerminal (with try/catch) outside of the
summaries.enabled guard and not chained to the summary await, ensuring failures
in the summary path do not prevent the title refresh and still log errors via
logger.warn on failure.
In `@apps/desktop/src/renderer/components/run/processUtils.test.ts`:
- Around line 80-90: The two tests for hasInspectableProcessOutput are
duplicates because both call makeRuntime({ status: "stopped", lastExitCode: 0
}); update one of the tests (e.g., the one titled "returns true for stopped
process with lastExitCode") to use a non-zero exit code (such as lastExitCode:
1) so you cover both zero and non-zero cases; locate the tests referencing
hasInspectableProcessOutput and makeRuntime and change the lastExitCode value in
one test accordingly.
In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx`:
- Around line 86-91: The input's onBlur unconditionally calls commitRename
causing a rename after Escape/Enter; in SessionContextMenu update the input
handlers (onKeyDown, onBlur, and onClose/commitRename) to guard against
duplicate or unintended commits by introducing a local ref/state (e.g.,
commitOrCancelledRef) that you set when Enter commits or Escape cancels (or when
onClose runs) and then have onBlur check that ref before calling commitRename;
ensure the flag prevents commitRename if cancellation occurred and is reset
appropriately after handling to allow future edits.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 207-211: The rename handler sets manuallyNamed: true but that flag
is dropped in the IPC parsing layer; update parseAgentChatUpdateSessionArgs in
syncRemoteCommandService to accept and forward the manuallyNamed boolean
(alongside sessionId and title) so that the agentChat.updateSession payload
includes it and the session update logic receives/uses manuallyNamed; update any
related type/interface handling inside parseAgentChatUpdateSessionArgs to
include the new field and ensure downstream code that consumes its return value
passes the flag through to the session update flow (refer to
parseAgentChatUpdateSessionArgs and agentChat.updateSession).
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts`:
- Around line 109-115: The test hardcodes the modes array duplicating the
AgentChatPermissionMode values which can drift; change the loop to derive modes
from a single source of truth by importing and iterating over a PERMISSION_MODES
constant (or the equivalent exported array) instead of the hardcoded list so new
modes are automatically covered; update the test that calls
buildTrackedCliStartupCommand({ provider: "claude" | "codex", permissionMode:
mode }) to iterate PERMISSION_MODES and keep assertions the same, referencing
AgentChatPermissionMode and buildTrackedCliStartupCommand to locate the test and
the type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97c4adba-b614-4c34-a4e5-9efeeb3565f3
⛔ Files ignored due to path filters (2)
docs/features/CHAT.mdis excluded by!docs/**docs/features/TERMINALS_AND_SESSIONS.mdis excluded by!docs/**
📒 Files selected for processing (15)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.test.tsapps/desktop/src/main/services/prs/prRebaseResolver.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/run/processUtils.test.tsapps/desktop/src/renderer/components/run/processUtils.tsapps/desktop/src/renderer/components/settings/AiFeaturesSection.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/shared/types/chat.ts
Owner
Author
|
@codex review |
![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: ebdf392e1d
ℹ️ 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".
…vergence, PTY title refresh - Persist manuallyNamed flag across session rehydration (serialize/deserialize in PersistedChatState) - Forward manuallyNamed through syncRemoteCommandService.parseAgentChatUpdateSessionArgs - Re-check manuallyNamed after async auto-title generation to prevent clobbering manual renames - Fix git divergence detection: default to "pull" for genuine divergence, "force_push_lease" only when upstream is ancestor of HEAD (post-rebase) - Decouple PTY title refresh from AI summaries toggle so titles refresh even when summaries are disabled - Fix Escape/blur rename: track cancellation state so onBlur doesn't commit after Escape - Fix laneLaunchContext test: assert on the same mock instance passed to resolveLaneLaunchContext - Fix duplicate processUtils test: use non-zero exit code for distinct test coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/desktop/src/main/services/pty/ptyService.ts (2)
354-381:⚠️ Potential issue | 🟠 MajorCatch AI summary failures locally before the refresh block.
Line 372 still sits in the same async callback as the title-refresh path. If
summarizeTerminal()rejects there, the callback exits before Line 383 runs, sorefreshOnCompleteis skipped again. Wrap the summary call in its owntry/catchand log that failure there instead of letting the outer.catch()short-circuit the rest of the completion flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 354 - 381, The AI summary call (aiIntegrationService!.summarizeTerminal) and subsequent sessionService.setSummary should be isolated in their own try/catch so a rejection doesn't short-circuit the rest of the completion flow (including refreshOnComplete); wrap the await aiIntegrationService!.summarizeTerminal(...) and the text handling in a try/catch, log the error locally (using the existing logger), and allow the outer completion logic to continue so refreshOnComplete still runs even if summarization fails.
410-412:⚠️ Potential issue | 🟠 MajorVerify the PTY auto-title writes still respect
manuallyNamed.Lines 412 and 863 still call
sessionService.updateMeta({ sessionId, title })after an async AI round-trip with no local re-check. If the manual-name guard only lives inagentChatService.updateSession, a rename made before or during the request can still be clobbered here.Use this to confirm whether
sessionService.updateMetaalready enforces the guard centrally:#!/bin/bash set -euo pipefail rg -n -C4 '\bmanuallyNamed\b|\bupdateMeta\s*\(' \ apps/desktop/src/main/services/pty/ptyService.ts \ apps/desktop/src/main/services/chat/agentChatService.ts \ apps/desktop/src/main/services/sessions/sessionService.tsExpected result: if
apps/desktop/src/main/services/sessions/sessionService.tsshowsupdateMetawritingtitlewithout consultingmanuallyNamed, this concern is confirmed and the PTY paths need the same post-awaitguard.Also applies to: 861-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 410 - 412, The PTY auto-title path calls sessionService.updateMeta({ sessionId, title }) after an async AI round-trip and can clobber a title set by the user; verify whether sessionService.updateMeta already checks the manuallyNamed flag and if it does not, either (A) add a post-await guard in ptyService (where finalTitle is computed) by reloading the session meta (via sessionService.get/getMeta or similar) and only call sessionService.updateMeta when manuallyNamed is false, or (B) enforce the guard centrally inside sessionService.updateMeta so all callers (including agentChatService.updateSession and ptyService.updateMeta calls at the end of the AI round-trip) are blocked from overwriting manuallyNamed titles. Ensure you reference sessionService.updateMeta, manuallyNamed, and agentChatService.updateSession when making the change.apps/desktop/src/main/services/chat/agentChatService.ts (1)
3186-3186:⚠️ Potential issue | 🟠 MajorV2 still can't distinguish legacy sessions from explicit
false.
persistChatState()omitsmanuallyNamedwhen it's false, and hydration only trustspersisted?.manuallyNamed === true. That means an older manually renamed chat is still indistinguishable from a v2 session withmanuallyNamed === false, so the first post-upgrade rehydrate can still leave a user-defined title vulnerable to final auto-titling. Persist the boolean explicitly in v2, or preserve enough legacy-version signal to infertruefrom an existing non-default title during hydration.Also applies to: 3302-3302, 4131-4131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` at line 3186, persistChatState() currently omits the manuallyNamed flag when false (using ...(managed.manuallyNamed ? { manuallyNamed: true } : {})), and hydration checks only persisted?.manuallyNamed === true, making legacy sessions with an explicit false indistinguishable from older manually-renamed chats; fix by explicitly persisting the boolean in persistChatState() (always include manuallyNamed: Boolean(managed.manuallyNamed)) or, alternatively, update the hydration logic (where persisted?.manuallyNamed is checked) to treat an undefined manuallyNamed as true if the stored title differs from the default/generated title; update references to persistChatState(), managed.manuallyNamed, and the hydration check persisted?.manuallyNamed === true accordingly so v2 sessions reliably differentiate legacy renamed chats from explicit false.apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx (1)
61-93:⚠️ Potential issue | 🟠 MajorUse a single “finalized” guard and reset it on menu changes.
Current logic only blocks post-Escape commits. On Line 90, Enter can commit, then Line 93 blur can commit again. Also,
cancelledRef.currentis never reset when menu reopens, so later rename attempts can be ignored.Suggested fix
- const cancelledRef = useRef(false); + const renameFinalizedRef = useRef(false); useEffect(() => { setRenaming(false); setDraft(""); + renameFinalizedRef.current = false; }, [menu]); const commitRename = () => { - if (cancelledRef.current) return; + if (renameFinalizedRef.current) return; + renameFinalizedRef.current = true; const trimmed = draft.trim(); if (trimmed.length > 0) { onRename(session.id, trimmed); } onClose(); }; + + const cancelRename = () => { + if (renameFinalizedRef.current) return; + renameFinalizedRef.current = true; + onClose(); + }; @@ onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); commitRename(); } - if (e.key === "Escape") { e.preventDefault(); cancelledRef.current = true; onClose(); } + if (e.key === "Escape") { e.preventDefault(); cancelRename(); } }}As per coding guidelines:
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx` around lines 61 - 93, The rename flow can commit multiple times because only cancelledRef is used and never reset; introduce a single "finalized" guard (e.g., finalizedRef.current) checked by commitRename, the onKeyDown Escape handler, and the onBlur handler so commitRename returns early if finalized, set finalizedRef.current = true whenever you call onRename or when Escape is handled to prevent duplicate commits, and reset finalizedRef.current = false whenever the menu/renaming state opens (e.g., in an effect watching renaming or when rendering the menu) so future opens aren't blocked; update references to cancelledRef.current usage to use this unified finalized guard and ensure inputRef focus behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx`:
- Around line 52-60: Move the useRef declaration so hooks always run regardless
of the early return: declare cancelledRef = useRef(false) (and any other hooks)
before the early return that checks "if (!menu) return null;". Update the
SessionContextMenu component so cancelledRef (and any other hook calls) are
placed above evaluating "menu" to ensure consistent hook order across renders.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Line 3186: persistChatState() currently omits the manuallyNamed flag when
false (using ...(managed.manuallyNamed ? { manuallyNamed: true } : {})), and
hydration checks only persisted?.manuallyNamed === true, making legacy sessions
with an explicit false indistinguishable from older manually-renamed chats; fix
by explicitly persisting the boolean in persistChatState() (always include
manuallyNamed: Boolean(managed.manuallyNamed)) or, alternatively, update the
hydration logic (where persisted?.manuallyNamed is checked) to treat an
undefined manuallyNamed as true if the stored title differs from the
default/generated title; update references to persistChatState(),
managed.manuallyNamed, and the hydration check persisted?.manuallyNamed === true
accordingly so v2 sessions reliably differentiate legacy renamed chats from
explicit false.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 354-381: The AI summary call
(aiIntegrationService!.summarizeTerminal) and subsequent
sessionService.setSummary should be isolated in their own try/catch so a
rejection doesn't short-circuit the rest of the completion flow (including
refreshOnComplete); wrap the await aiIntegrationService!.summarizeTerminal(...)
and the text handling in a try/catch, log the error locally (using the existing
logger), and allow the outer completion logic to continue so refreshOnComplete
still runs even if summarization fails.
- Around line 410-412: The PTY auto-title path calls sessionService.updateMeta({
sessionId, title }) after an async AI round-trip and can clobber a title set by
the user; verify whether sessionService.updateMeta already checks the
manuallyNamed flag and if it does not, either (A) add a post-await guard in
ptyService (where finalTitle is computed) by reloading the session meta (via
sessionService.get/getMeta or similar) and only call sessionService.updateMeta
when manuallyNamed is false, or (B) enforce the guard centrally inside
sessionService.updateMeta so all callers (including
agentChatService.updateSession and ptyService.updateMeta calls at the end of the
AI round-trip) are blocked from overwriting manuallyNamed titles. Ensure you
reference sessionService.updateMeta, manuallyNamed, and
agentChatService.updateSession when making the change.
In `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx`:
- Around line 61-93: The rename flow can commit multiple times because only
cancelledRef is used and never reset; introduce a single "finalized" guard
(e.g., finalizedRef.current) checked by commitRename, the onKeyDown Escape
handler, and the onBlur handler so commitRename returns early if finalized, set
finalizedRef.current = true whenever you call onRename or when Escape is handled
to prevent duplicate commits, and reset finalizedRef.current = false whenever
the menu/renaming state opens (e.g., in an effect watching renaming or when
rendering the menu) so future opens aren't blocked; update references to
cancelledRef.current usage to use this unified finalized guard and ensure
inputRef focus behavior remains unchanged.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfa060e5-d10f-44d7-8706-4ed497f3deb7
📒 Files selected for processing (7)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/lanes/laneLaunchContext.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/renderer/components/run/processUtils.test.tsapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/run/processUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/services/git/gitOperationsService.ts
- apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts
apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
Outdated
Show resolved
Hide resolved
…bering - Move useRef(cancelledRef→finalizedRef) above early return in SessionContextMenu to satisfy React rules-of-hooks lint rule - Replace cancelledRef with a unified finalizedRef guard so commitRename cannot fire twice (Enter+blur or Escape+blur) - Always persist manuallyNamed as an explicit boolean in agentChatService so v2 sessions reliably distinguish false from undefined - Wrap AI-enhanced summary call in ptyService in its own try/catch so a rejection does not short-circuit the title-refresh-on-complete flow - Add post-await guard in both ptyService auto-title paths: reload the session and skip updateMeta when the title changed (user renamed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
9215-9227:⚠️ Potential issue | 🟡 MinorBlank title resets still leave auto-titling disabled.
Lines 9216-9220 coerce
null/blank titles back to the provider default, but Lines 9221-9227 only honormanuallyNamedwhen it istrue, or whentitle === undefined. The sync parser already preservesmanuallyNamed: false, so a caller cannot atomically reset the title and clear the manual-name lock. That leaves sessions on the default title whilemaybeAutoTitleSession()keeps skipping future summary titles.💡 Proposed fix
if (title !== undefined) { const normalizedTitle = String(title ?? "").trim(); + const hasExplicitTitle = normalizedTitle.length > 0; sessionService.updateMeta({ sessionId, - title: normalizedTitle.length ? normalizedTitle : defaultChatSessionTitle(managed.session.provider), + title: hasExplicitTitle ? normalizedTitle : defaultChatSessionTitle(managed.session.provider), }); - if (manuallyNamed === true) { - managed.manuallyNamed = true; + if (manuallyNamed !== undefined) { + managed.manuallyNamed = manuallyNamed && hasExplicitTitle; + } else if (!hasExplicitTitle) { + managed.manuallyNamed = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 9215 - 9227, The code updates the session title but only sets managed.manuallyNamed when manuallyNamed === true or when title === undefined, preventing callers from clearing the manual-name lock when sending a blank/null title; change the logic so managed.manuallyNamed is updated whenever the caller provides a manuallyNamed value (i.e., if (manuallyNamed !== undefined) managed.manuallyNamed = manuallyNamed), while keeping the existing title normalization via sessionService.updateMeta and defaultChatSessionTitle; reference sessionService.updateMeta, defaultChatSessionTitle, managed.manuallyNamed, title, manuallyNamed, and maybeAutoTitleSession when making the change.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/pty/ptyService.ts (1)
242-252: Consider adding typed interface for session intelligence config.Both helpers use
as anytype assertions to access config properties (autoTitleEnabled,autoTitleRefreshOnCompleteat line 393). This works but bypasses TypeScript's safety checks.🔧 Suggested improvement
Define the expected shape in the config types rather than casting:
// In project config types interface ChatConfig { // existing fields... autoTitleEnabled?: boolean; autoTitleRefreshOnComplete?: boolean; }Then these helpers can access properties without
as any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 242 - 252, The helpers isTitleGenerationEnabled and resolveTitleModelId use unsafe casts (as any) when reading chat-related config; add a typed interface for the chat portion of project config (e.g., interface ChatConfig { autoTitleEnabled?: boolean; autoTitleRefreshOnComplete?: boolean; /* other existing fields */ }) in your project config types, update the return/type of projectConfigService.get().effective.ai or its Chat type to use this ChatConfig, and then remove the as any casts in isTitleGenerationEnabled and any other place (referencing getSessionIntelligence, projectConfigService, and the autoTitleEnabled/autoTitleRefreshOnComplete fields) so TypeScript enforces correct property access and avoids unsafe assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 9215-9227: The code updates the session title but only sets
managed.manuallyNamed when manuallyNamed === true or when title === undefined,
preventing callers from clearing the manual-name lock when sending a blank/null
title; change the logic so managed.manuallyNamed is updated whenever the caller
provides a manuallyNamed value (i.e., if (manuallyNamed !== undefined)
managed.manuallyNamed = manuallyNamed), while keeping the existing title
normalization via sessionService.updateMeta and defaultChatSessionTitle;
reference sessionService.updateMeta, defaultChatSessionTitle,
managed.manuallyNamed, title, manuallyNamed, and maybeAutoTitleSession when
making the change.
---
Nitpick comments:
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 242-252: The helpers isTitleGenerationEnabled and
resolveTitleModelId use unsafe casts (as any) when reading chat-related config;
add a typed interface for the chat portion of project config (e.g., interface
ChatConfig { autoTitleEnabled?: boolean; autoTitleRefreshOnComplete?: boolean;
/* other existing fields */ }) in your project config types, update the
return/type of projectConfigService.get().effective.ai or its Chat type to use
this ChatConfig, and then remove the as any casts in isTitleGenerationEnabled
and any other place (referencing getSessionIntelligence, projectConfigService,
and the autoTitleEnabled/autoTitleRefreshOnComplete fields) so TypeScript
enforces correct property access and avoids unsafe assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da90f631-835d-487a-9858-3ebbeec1db2c
📒 Files selected for processing (3)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Owner
Author
|
@codex fix the comments CodeRabbit left, and push commit here |
Summary
Testing
Also completed follow-up actions requested:
|
This was referenced Mar 31, 2026
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests