chat-ux: collapse thoughts + scroll fixes#190
Conversation
Phase 7 reference flow assumed a single --group target and the one-shot `asc publish testflight --group ... --wait`. Two problems in practice: 1. The command never asked the user which TestFlight group to target, so builds could land in a group with zero testers (invisible) or in the wrong audience (e.g. Public Beta when only Internal Testers has members). 2. The one-shot form races in local-build mode. `--wait` returns when processingState=VALID, but usesNonExemptEncryption is still None at that instant, and the subsequent add-groups fails with "Build is not in an externally assignable state" (asc 1.2.x). Changes: - Phase 7a: always pause for build number AND target group(s); enumerate groups + tester counts before asking; explain internal vs external. - Phase 7d: call out the race; do not use --group with --wait in local-build mode. - Phase 7e: safe sequenced recipe (upload+wait → resolve build id → answer encryption → add to groups). - Phase 7f: verify distribution explicitly — non-empty group, build linked to group, correct beta state flags. Don't trust autoNotify alone. - Execution mode: add group selection to the pause list. Co-Authored-By: Claude Opus 4.7 <[email protected]>
…child processes
Windows release jobs hung for 1h26m on validate:win:release because the
packaged Electron-as-node child (running packagedRuntimeSmoke.cjs)
kept event-loop handles alive after writing its JSON result. The
validator's runCommand helper had no timeout, so child.on("close")
never fired.
- packagedRuntimeSmoke.ts: call process.exit(0) on main success. The
catch path already exits; the success path relied on the event loop
emptying, which is not guaranteed on Windows (node-pty/conpty and the
Claude SDK can leave handles open).
- validate-win-artifacts.mjs: add a 3-minute default timeout to
runCommand, SIGKILL on expiry, reject with captured stdout/stderr.
Hangs in any other validator-spawned child (ade.cmd wrapper, installer
shim, the plain-node fallback branch) now fail fast with a clear
message instead of sitting until the 6-hour job timeout.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
ci.yml had no Windows coverage. A Windows-specific regression in apps/desktop/scripts/validate-win-artifacts.mjs or the packaged smoke could (and did, on v1.1.3) only surface at release time, where a hang costs a full Windows runner for the job timeout. Add a self-contained build-win job on windows-latest that runs the same dist:win pipeline release-core.yml uses (minus the upload step). Self-contained rather than cache-sharing with the Linux install job because Windows node_modules contain platform-specific native binaries (node-pty, cr-sqlite, onnxruntime-node) that can't share the Linux archive. Uses the built-in setup-node npm cache instead. Added to the ci-pass gate so a broken Windows pipeline blocks merge. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Now that Windows builds ship alongside macOS, the draft release carries both platforms' artifacts. Update /release to set expectations and gate the Phase 8 summary on a full asset checklist. - Phase 5: enumerate the expected macOS and Windows asset set after the draft is created. - Phase 8: verify every expected asset is present before flipping the draft. Refuse to report `done` on an asymmetric release — shipping mac-only (or win-only) would break electron-updater consumers on the missing platform. Co-Authored-By: Claude Opus 4.7 <[email protected]>
…ck keys
Assistant bubbles were rendering with doubled text (one bubble, content
emitted verbatim twice, no separator) every few messages.
Root cause: Claude stream processing has two emission paths per turn —
streaming deltas (content_block_delta → text_delta) and the final
assistant snapshot. streamedClaudeTextContentKeys/ThinkingContentKeys
dedup the snapshot against streamed content using claudeDedupeKey.
The key uses `currentClaudeStreamMessageId` on the delta path and
`assistantMessageId` on the snapshot path, with a shared
`${turnId}:b${boundary}` fallback when id is null.
When content_block_delta arrived before message_start (SDK ordering
variance / mocks / cached responses), deltas stored fallback keys
`${turnId}:b${N}:${idx}`. The snapshot then arrived with a real
assistantMessageId and computed `${msg_abc}:${idx}` — missed the set,
re-emitted the text. ensureAssistantMessageId glued both events onto
the same activeAssistantMessageId, so the renderer concatenated them
into one bubble = visible doubling.
Fix: at snapshot-time lookup, check both the real-id key and the
id-less fallback key. Mirrored for thinking blocks. Delta-side storage
stays as-is.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
…ion when deltas streamed
Codex runtime has two emission paths for command execution and file
changes:
1. `item/.../outputDelta` streams incremental chunks (emits the delta
as-is, not cumulative)
2. `item/.../completed` snapshots the full aggregated content
The renderer merges same-itemId events via `mergeStreamingText`, which
uses `incoming.startsWith(existing)` to decide whether a snapshot
replaces accumulated deltas or appends to them. That heuristic fails
when the snapshot prefix doesn't exactly match the delta accumulation —
most reliably for file diffs, where the full snapshot carries
`--- a/file\n+++ b/file` headers that outputDelta chunks never include.
The snapshot then gets appended onto the accumulated deltas, producing
visible doubled content in the rendered command output / file diff.
Track which itemIds streamed deltas via two new runtime sets
(`streamedCommandItemIds`, `streamedFileChangeItemIds`). On the
completed snapshot path, if the item streamed, emit the event with
empty output/diff — the renderer's `mergeStreamingText` treats empty
incoming as a no-op and keeps the accumulated delta content while the
status transition ("running" → "completed"/"failed") still applies.
Clear both sets in the existing runtime-state reset path alongside
`commandOutputByItemId` / `fileDeltaByItemId`.
Found by /audit follow-up after fixing the analogous Claude
stream-vs-snapshot dedup bug. Same bug class, different runtime.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
Captures in-progress branch state across desktop + iOS chat surfaces (composer, message list, reasoning card, mention input), sync/session plumbing for command output, model registry tweaks, claude/codex slash command discovery updates, and a small ade-cli linear-services tweak. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
|
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 22 minutes and 46 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 ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds session archiving capabilities for agent chats, upgrades default model references from GPT-5.4 to GPT-5.5, introduces slash-command discovery backed by markdown files for both Claude and Codex, refactors the chat composer to use a unified command menu, enhances terminal output rendering on iOS, and adds Windows smoke-build CI coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Here's a review of the PR changes: Scroll fixes (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| if first == 1 { | ||
| lines[row].removeFirst(min(column, lines[row].count)) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The CSI 1K handler uses removeFirst which shifts remaining array elements left, corrupting the positions of characters after the cursor. The standard behavior for ESC[1K is to blank characters from position 0 through the cursor column (inclusive) without shifting content, and the cursor position should remain unchanged.
// WorkNavigationAndTranscriptHelpers.swift
if first == 1 {
lines[row].removeFirst(min(column, lines[row].count))
column = 0
}For example, with line content ABCDEFGH and cursor at column 3, CSI 1K should produce DEFGH (blanking A-D, cursor stays at 3). The current code produces EFGH (shifts E-H left, cursor at 0). This affects display of any CLI output using CSI 1K, which is common in progress bars and status-line tools.
| if first == 1 { | |
| lines[row].removeFirst(min(column, lines[row].count)) | |
| if first == 1 { | |
| let count = min(column + 1, lines[row].count) | |
| for i in 0..<count { lines[row][i] = " " } | |
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/scripts/validate-win-artifacts.mjs (1)
186-213:⚠️ Potential issue | 🟡 MinorTimeout kill may leak grandchild processes when
useShell=trueon Windows.When the command is a
.cmd/.batwrapper,spawnusesshell: true(Line 192), sochildbecomes the shell process andchild.kill("SIGKILL")terminates only that shell on Windows — the grandchild (e.g.,node.exelaunched byade.cmd) can survive and consume resources even after timeout rejection. The Promise unblocks correctly, so this is a resource-leak concern rather than a correctness bug, but onwindows-latestrunners it can delay cleanup.Use
taskkill /T /F /PID <pid>on Windows when a shell is involved, or avoidshell: truefor direct commands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/validate-win-artifacts.mjs` around lines 186 - 213, The timeout handler can leak grandchild processes when useShell is true on Windows because child.kill("SIGKILL") only kills the shell; update the timer logic in validate-win-artifacts so that if process.platform === "win32" && useShell is true you run a taskkill tree kill (e.g., spawnSync or execFileSync "taskkill" with ['/T','/F','/PID', String(child.pid)]) to forcefully terminate the whole process tree, otherwise fall back to the existing child.kill("SIGKILL"); ensure you still build the same rejection Error with rendered/details and keep existing try/catch around kill attempts and clear the timer appropriately.apps/ios/ADE/Models/RemoteModels.swift (1)
1127-1161:⚠️ Potential issue | 🔴 Critical
archivedAtis missing fromencode(to:)— round-tripping will drop the archive state.You added
archivedAtto the stored properties,CodingKeys, and the decoder, but the customencode(to:)does not write it. Any re-encode of anAgentChatSession(persistence, forwarding to a peer, test fixtures, etc.) will silently lose the archival metadata that the rest of this PR depends on.🛠 Proposed fix
try container.encode(status, forKey: .status) try container.encodeIfPresent(idleSinceAt, forKey: .idleSinceAt) + try container.encodeIfPresent(archivedAt, forKey: .archivedAt) try container.encodeIfPresent(threadId, forKey: .threadId) try container.encodeIfPresent(requestedCwd, forKey: .requestedCwd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Models/RemoteModels.swift` around lines 1127 - 1161, The custom Encoder implementation for AgentChatSession (encode(to:)) omits the archivedAt property so serializing an instance will drop archive state; update encode(to:) to encodeIfPresent(archivedAt, forKey: .archivedAt) (using the existing CodingKeys enum which already includes archivedAt) alongside the other fields so round-trips preserve the archival metadata.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (2)
217-222:Remove-Itempaths won't resolve relative to repo root.
Remove-Item -Recurse -Force apps/desktop/release, apps/desktop/.cache -ErrorAction SilentlyContinuerelies on the step's current working directory being the repo root. GitHub Actions defaults to${{ github.workspace }}, so this works today, but the-ErrorAction SilentlyContinuealso masks any genuine failure (e.g., if the path ever changes). SinceNew-Itemon the next line uses a matching relative path the behavior is fine — just flagging that if someone later adds aworking-directory:to this step, the silent reset would quietly become a no-op. A minor hardening would be to use${{ github.workspace }}\apps\desktop\...here as well, matching the env-var style used in theBuild and validatestep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 217 - 222, The Remove-Item command is using relative paths which can break if the step's working directory changes and the -ErrorAction SilentlyContinue masks failures; update the PowerShell commands (the Remove-Item and New-Item invocations in the "Reset release output" step) to use the GitHub workspace environment variable (github.workspace) to build absolute paths for apps/desktop/release and apps/desktop/.cache so they resolve correctly regardless of working-directory changes and avoid silently no-op failures.
199-227: Consider gatingbuild-winon the cheaper Linux checks.As written,
build-winstarts in parallel withtypecheck-desktop/lint-desktop/test-desktop. A PR with a trivial TS error will still consume ~10–20 minutes ofwindows-latesttime before failing the Windows dist build for the same reason. Addingneeds: [typecheck-desktop, lint-desktop](and possiblytest-desktop) would short-circuit obvious failures and save runner minutes, at the cost of slightly delaying the Windows signal on passing PRs.No functional issue — just a CI-cost consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 199 - 227, The build-win job is starting in parallel and should be gated to avoid wasting Windows runner time; update the build-win job (named build-win) to include a needs: field referencing the earlier desktop validation jobs (e.g., needs: [typecheck-desktop, lint-desktop] and optionally test-desktop) so the Windows build only runs after those checks pass.apps/desktop/src/renderer/components/missions/missionHelpers.ts (1)
252-252: Consider centralizing the fallback model ID constant.Line 252 is correct, but this literal appears in multiple places and will drift on future model bumps. Consider exporting a shared fallback constant (e.g., from model registry/profiles) and reusing it here and related policy/tool strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/missions/missionHelpers.ts` at line 252, The code uses a hardcoded fallback model ID in the modelId assignment (modelId: getDefaultModelDescriptor("codex")?.id ?? "openai/gpt-5.5-codex") which is duplicated elsewhere; extract that literal into a shared exported constant (e.g., DEFAULT_FALLBACK_MODEL_ID) in the central model registry or profiles module, export it, and replace this literal here and in other locations with that constant, keeping the current getDefaultModelDescriptor("codex")?.id ?? DEFAULT_FALLBACK_MODEL_ID logic so behavior stays the same; update any related policy/tool strings to reuse the same exported constant.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
3429-3434: Consider replacing right-click +window.confirmwith an explicit archive action.Archive is currently hidden behind context menu only, which is harder to discover/use on touch and keyboard-first workflows.
🤖 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 3429 - 3434, The archive action is only accessible via the onContextMenu handler (right-click) using window.confirm which is not discoverable for touch/keyboard users; add an explicit, discoverable archive control (e.g., an Archive button or menu item) next to the chat title that calls handleArchiveChat(session.sessionId) and uses a proper modal/confirm UI instead of window.confirm, keeping the same confirmation text that references title and the same archived behavior; remove or retain the onContextMenu as a secondary pathway but ensure keyboard and touch users can trigger the archive via the new control and that it reuses the same confirmation and handler logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/release.md:
- Line 447: Update the echo statement that prints group, testers and build
presence: change the label "build5_present" to "build_present" so it matches the
intended variable name; edit the line containing echo "group=$gid testers=$count
build5_present=$([ -n "$members" ] && echo yes || echo no)" and replace the
label accordingly while keeping the command substitution and variables ($gid,
$count, $members) unchanged.
In `@apps/desktop/src/main/packagedRuntimeSmoke.ts`:
- Around line 121-131: The code currently calls process.exit(0) immediately
after main() resolves which can truncate buffered stdout writes (the JSON
written by main via process.stdout.write); modify the exit path to flush stdout
before exiting by ensuring any pending process.stdout.write completes (if write
returns false wait for the 'drain' event or use the write callback/promise) and
only then call process.exit(0); likewise for the error branch ensure the stderr
write completes before calling process.exit(1). Locate the main() resolution
handlers in packagedRuntimeSmoke.ts and update the success and error handlers to
await/attach completion to process.stdout.write/process.stderr.write before
exiting.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 11026-11040: The code expands file-backed prompt files even when a
runtime/provider-native slash command with the same name exists; change the
expansion guard so runtime/provider-native commands take precedence.
Specifically, when computing expandedClaudeSlashCommand and
expandedCodexSlashCommand and when choosing promptText, ensure you detect and
prefer runtime/provider-native slash commands (use the existing
providerSlashCommand and the merged slash command source from getSlashCommands
or equivalent flag) and only fall back to file-backed expansion if the slash
command is neither a built-in nor a runtime/provider-native command; update the
conditions around CLAUDE_BUILT_IN_SLASH_COMMAND_NAMES,
CODEX_BUILT_IN_SLASH_COMMAND_NAMES, resolveClaudeSlashCommandInvocation,
resolveCodexSlashCommandInvocation, and composeLaunchDirectives accordingly so
runtime commands are executed instead of local prompt files with the same name.
In `@apps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.ts`:
- Around line 59-61: normalizeSlashCommandName() lowercases and slugifies
filenames causing lookups to fail on case-sensitive filesystems because
resolution rebuilds a literal path from the normalized name; fix by making
discovery preserve the original file basename (or map normalized -> actual
filename) instead of deriving the lookup path from the normalized string. Update
the discovery code that calls normalizeSlashCommandName() (and the resolution
logic referenced around lines 120-134) to store a mapping like normalizedName ->
originalFilename or originalPath and use that originalFilename/originalPath for
resolution/loading so filenames with uppercase letters, spaces, or punctuation
resolve correctly.
- Around line 213-221: The command parser currently builds promptText from the
template body and replaces $ARGUMENTS, but if the template omits $ARGUMENTS any
user-supplied arguments in argumentsText are dropped; update the promptText
construction in the block that returns { name, argumentsText, promptText } so
that if body.includes("$ARGUMENTS") you replace as before, otherwise if
argumentsText is non-empty you append a single-space-separated argumentsText to
the trimmed body (ensuring no duplicate spaces); reference the existing symbols
name, argumentsText, promptText, stripFrontmatter and the try block reading
commandFile when making this change.
In `@apps/desktop/src/main/services/chat/codexSlashCommandDiscovery.ts`:
- Around line 32-35: The normalized names produced by normalizeSlashCommandName
cannot be used directly to find on-disk files because filenames may be
non-canonical (e.g., "My Prompt.md"); update discovery to record a mapping from
the normalized command (from normalizeSlashCommandName) to the actual file path
returned by discovery and use that mapping in resolvePromptFile (or its caller)
to look up the real file instead of assuming the normalized path exists;
specifically, modify the code that builds the command list (the function that
calls normalizeSlashCommandName and returns discovered commands) to store {
normalizedName => realFilePath } and change resolvePromptFile (or the resolver
code referenced around lines 77-91) to consult that mapping, falling back to a
case-insensitive / punctuation-stripped search of the discovered filenames if
needed.
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 558-571: archiveSession and unarchiveSession currently always
return true and call emitChanged even when no terminal_sessions row exists;
change them to verify the update actually affected a row before emitting or
returning success by checking the database run result (e.g., the statement
result's changes/count or use a preceding SELECT to confirm existence).
Specifically, update the implementations referencing db.run("update
terminal_sessions ...", ...) to inspect the result (changes > 0) and only call
emitChanged({ sessionId: trimmed, reason: "meta-updated" }) and return true when
a row was modified; otherwise return false (and do not emit). Keep the same
parameter handling and trimmed variable names.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 644-650: handleSlashSelect currently falls through and sets the
draft to "/clear" when a local /clear command is selected but onClearEvents is
undefined; change the logic in handleSlashSelect so that for cmd.command ===
"/clear" && cmd.source === "local" you only call onClearEvents() and clear the
draft when onClearEvents is present, and otherwise return early (do not call
onDraftChange or fall through to the generic draft path). Update the handler
surrounding the local /clear branch (handleSlashSelect, using onClearEvents and
onDraftChange) to explicitly ignore or reject unsupported local commands when
their handler is missing.
- Around line 1917-1922: The slash command menu is kept open for drafts like
"/cmd foo" which prevents Enter/Tab submission; change the branch in
AgentChatComposer where you check val.startsWith("/") (before calling
setCommandMenuTrigger) to only treat it as a trigger when the draft is exactly
the slash token (e.g., val === "/"), and otherwise clear the trigger (call
setCommandMenuTrigger(null) or don't set it) so keyboard submission
(handleKeyDown) is not blocked for commands with arguments; update the condition
and ensure setCommandMenuAnchor is only set when the trigger is active.
In `@apps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swift`:
- Around line 199-204: The CSI "K" (case "K") branch incorrectly shifts text
left by calling lines[row].removeFirst(...) and setting column = 0; instead
implement erase-in-line semantics for parameter first == 1 by replacing the
prefix up to min(column, lines[row].count) with the same number of space
characters in-place and do not change the cursor position. Locate the case "K"
block (ensureCursor(), lines[row], column, first) and change the logic so it
writes spaces into the range 0..<min(column, lines[row].count) (preserving
string length) and remove the assignment column = 0.
In `@apps/ios/ADE/Views/Work/WorkRootScreen.swift`:
- Around line 68-70: The current local.union(remote) lets IDs still present in
archivedSessionIdsStorage override remote unarchive state; change the logic to
prefer remote for any session present in chatSummaries by removing known IDs
from local before merging: compute local as
Set(archivedSessionIdsStorage.split(separator: "\n").map(String.init)), compute
remote as Set(chatSummaries.values.compactMap { $0.archivedAt == nil ? nil :
$0.sessionId }), compute localUnknown =
local.subtracting(Set(chatSummaries.keys)), and return
remote.union(localUnknown) so only legacy/unknown IDs from
archivedSessionIdsStorage are kept.
---
Outside diff comments:
In `@apps/desktop/scripts/validate-win-artifacts.mjs`:
- Around line 186-213: The timeout handler can leak grandchild processes when
useShell is true on Windows because child.kill("SIGKILL") only kills the shell;
update the timer logic in validate-win-artifacts so that if process.platform ===
"win32" && useShell is true you run a taskkill tree kill (e.g., spawnSync or
execFileSync "taskkill" with ['/T','/F','/PID', String(child.pid)]) to
forcefully terminate the whole process tree, otherwise fall back to the existing
child.kill("SIGKILL"); ensure you still build the same rejection Error with
rendered/details and keep existing try/catch around kill attempts and clear the
timer appropriately.
In `@apps/ios/ADE/Models/RemoteModels.swift`:
- Around line 1127-1161: The custom Encoder implementation for AgentChatSession
(encode(to:)) omits the archivedAt property so serializing an instance will drop
archive state; update encode(to:) to encodeIfPresent(archivedAt, forKey:
.archivedAt) (using the existing CodingKeys enum which already includes
archivedAt) alongside the other fields so round-trips preserve the archival
metadata.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 217-222: The Remove-Item command is using relative paths which can
break if the step's working directory changes and the -ErrorAction
SilentlyContinue masks failures; update the PowerShell commands (the Remove-Item
and New-Item invocations in the "Reset release output" step) to use the GitHub
workspace environment variable (github.workspace) to build absolute paths for
apps/desktop/release and apps/desktop/.cache so they resolve correctly
regardless of working-directory changes and avoid silently no-op failures.
- Around line 199-227: The build-win job is starting in parallel and should be
gated to avoid wasting Windows runner time; update the build-win job (named
build-win) to include a needs: field referencing the earlier desktop validation
jobs (e.g., needs: [typecheck-desktop, lint-desktop] and optionally
test-desktop) so the Windows build only runs after those checks pass.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 3429-3434: The archive action is only accessible via the
onContextMenu handler (right-click) using window.confirm which is not
discoverable for touch/keyboard users; add an explicit, discoverable archive
control (e.g., an Archive button or menu item) next to the chat title that calls
handleArchiveChat(session.sessionId) and uses a proper modal/confirm UI instead
of window.confirm, keeping the same confirmation text that references title and
the same archived behavior; remove or retain the onContextMenu as a secondary
pathway but ensure keyboard and touch users can trigger the archive via the new
control and that it reuses the same confirmation and handler logic.
In `@apps/desktop/src/renderer/components/missions/missionHelpers.ts`:
- Line 252: The code uses a hardcoded fallback model ID in the modelId
assignment (modelId: getDefaultModelDescriptor("codex")?.id ??
"openai/gpt-5.5-codex") which is duplicated elsewhere; extract that literal into
a shared exported constant (e.g., DEFAULT_FALLBACK_MODEL_ID) in the central
model registry or profiles module, export it, and replace this literal here and
in other locations with that constant, keeping the current
getDefaultModelDescriptor("codex")?.id ?? DEFAULT_FALLBACK_MODEL_ID logic so
behavior stays the same; update any related policy/tool strings to reuse the
same exported constant.
🪄 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: e894692b-b0f3-4b7a-bbe1-3781cd93fb4a
⛔ Files ignored due to path filters (1)
.claude/scheduled_tasks.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.claude/commands/audit.md.claude/commands/release.md.github/workflows/ci.ymlapps/ade-cli/README.mdapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/packagedRuntimeSmoke.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/claudeModelUtils.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.test.tsapps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.tsapps/desktop/src/main/services/chat/codexSlashCommandDiscovery.test.tsapps/desktop/src/main/services/chat/codexSlashCommandDiscovery.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/missions/phaseEngine.tsapps/desktop/src/main/services/orchestrator/executionPolicy.test.tsapps/desktop/src/main/services/orchestrator/executionPolicy.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatCommandMenu.tsxapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/missions/missionHelpers.tsapps/desktop/src/renderer/components/missions/missionPhaseDefaults.tsapps/desktop/src/renderer/components/shared/MentionInput.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/modelProfiles.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatSheet.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkReasoningCard.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ExportOptions.auto.plist
| void main().then( | ||
| () => { | ||
| // Force a clean exit even if node-pty/Claude SDK/Codex left event-loop handles open. | ||
| // Without this, the packaged Electron-as-node child can hang forever after writing JSON to stdout. | ||
| process.exit(0); | ||
| }, | ||
| (error) => { | ||
| process.stderr.write(error instanceof Error ? (error.stack ?? error.message) : String(error)); | ||
| process.exit(1); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/main/packagedRuntimeSmoke.tsRepository: arul28/ADE
Length of output: 5271
Potential stdout truncation when forcing process.exit(0).
process.exit() terminates synchronously without flushing buffered writes. Since main() writes JSON via process.stdout.write() on line 109 without checking the return value, the payload consumed by validate-win-artifacts.mjs could be truncated on Windows if the write buffer becomes full, causing downstream JSON.parse() to fail.
Consider flushing stdout before exiting:
🔧 Proposed fix
void main().then(
() => {
- // Force a clean exit even if node-pty/Claude SDK/Codex left event-loop handles open.
- // Without this, the packaged Electron-as-node child can hang forever after writing JSON to stdout.
- process.exit(0);
+ // Force a clean exit even if node-pty/Claude SDK/Codex left event-loop handles open.
+ // Without this, the packaged Electron-as-node child can hang forever after writing JSON to stdout.
+ // Flush stdout first so Windows pipe consumers receive the full JSON payload before we tear down.
+ if (process.stdout.writableNeedDrain) {
+ process.stdout.once("drain", () => process.exit(0));
+ } else {
+ process.exit(0);
+ }
},
(error) => {
- process.stderr.write(error instanceof Error ? (error.stack ?? error.message) : String(error));
- process.exit(1);
+ const message = error instanceof Error ? (error.stack ?? error.message) : String(error);
+ process.stderr.write(message, () => process.exit(1));
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void main().then( | |
| () => { | |
| // Force a clean exit even if node-pty/Claude SDK/Codex left event-loop handles open. | |
| // Without this, the packaged Electron-as-node child can hang forever after writing JSON to stdout. | |
| process.exit(0); | |
| }, | |
| (error) => { | |
| process.stderr.write(error instanceof Error ? (error.stack ?? error.message) : String(error)); | |
| process.exit(1); | |
| }, | |
| ); | |
| void main().then( | |
| () => { | |
| // Force a clean exit even if node-pty/Claude SDK/Codex left event-loop handles open. | |
| // Without this, the packaged Electron-as-node child can hang forever after writing JSON to stdout. | |
| // Flush stdout first so Windows pipe consumers receive the full JSON payload before we tear down. | |
| if (process.stdout.writableNeedDrain) { | |
| process.stdout.once("drain", () => process.exit(0)); | |
| } else { | |
| process.exit(0); | |
| } | |
| }, | |
| (error) => { | |
| const message = error instanceof Error ? (error.stack ?? error.message) : String(error); | |
| process.stderr.write(message, () => process.exit(1)); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/packagedRuntimeSmoke.ts` around lines 121 - 131, The
code currently calls process.exit(0) immediately after main() resolves which can
truncate buffered stdout writes (the JSON written by main via
process.stdout.write); modify the exit path to flush stdout before exiting by
ensuring any pending process.stdout.write completes (if write returns false wait
for the 'drain' event or use the write callback/promise) and only then call
process.exit(0); likewise for the error branch ensure the stderr write completes
before calling process.exit(1). Locate the main() resolution handlers in
packagedRuntimeSmoke.ts and update the success and error handlers to
await/attach completion to process.stdout.write/process.stderr.write before
exiting.
| archiveSession(sessionId: string, archivedAt: string = new Date().toISOString()): boolean { | ||
| const trimmed = sessionId.trim(); | ||
| if (!trimmed) return false; | ||
| db.run("update terminal_sessions set archived_at = coalesce(archived_at, ?) where id = ?", [archivedAt, trimmed]); | ||
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | ||
| return true; | ||
| }, | ||
|
|
||
| unarchiveSession(sessionId: string): boolean { | ||
| const trimmed = sessionId.trim(); | ||
| if (!trimmed) return false; | ||
| db.run("update terminal_sessions set archived_at = null where id = ?", [trimmed]); | ||
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | ||
| return true; |
There was a problem hiding this comment.
archiveSession/unarchiveSession can report success for non-existent sessions.
Both methods return true and emit meta-updated even when id does not exist. That makes no-op updates look successful to callers and downstream sync/UI flows.
Suggested fix
archiveSession(sessionId: string, archivedAt: string = new Date().toISOString()): boolean {
const trimmed = sessionId.trim();
if (!trimmed) return false;
+ const existing = db.get<{ present: number }>(
+ "select 1 as present from terminal_sessions where id = ? limit 1",
+ [trimmed],
+ );
+ if (!existing) return false;
db.run("update terminal_sessions set archived_at = coalesce(archived_at, ?) where id = ?", [archivedAt, trimmed]);
emitChanged({ sessionId: trimmed, reason: "meta-updated" });
return true;
},
unarchiveSession(sessionId: string): boolean {
const trimmed = sessionId.trim();
if (!trimmed) return false;
+ const existing = db.get<{ present: number }>(
+ "select 1 as present from terminal_sessions where id = ? limit 1",
+ [trimmed],
+ );
+ if (!existing) return false;
db.run("update terminal_sessions set archived_at = null where id = ?", [trimmed]);
emitChanged({ sessionId: trimmed, reason: "meta-updated" });
return true;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| archiveSession(sessionId: string, archivedAt: string = new Date().toISOString()): boolean { | |
| const trimmed = sessionId.trim(); | |
| if (!trimmed) return false; | |
| db.run("update terminal_sessions set archived_at = coalesce(archived_at, ?) where id = ?", [archivedAt, trimmed]); | |
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | |
| return true; | |
| }, | |
| unarchiveSession(sessionId: string): boolean { | |
| const trimmed = sessionId.trim(); | |
| if (!trimmed) return false; | |
| db.run("update terminal_sessions set archived_at = null where id = ?", [trimmed]); | |
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | |
| return true; | |
| archiveSession(sessionId: string, archivedAt: string = new Date().toISOString()): boolean { | |
| const trimmed = sessionId.trim(); | |
| if (!trimmed) return false; | |
| const existing = db.get<{ present: number }>( | |
| "select 1 as present from terminal_sessions where id = ? limit 1", | |
| [trimmed], | |
| ); | |
| if (!existing) return false; | |
| db.run("update terminal_sessions set archived_at = coalesce(archived_at, ?) where id = ?", [archivedAt, trimmed]); | |
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | |
| return true; | |
| }, | |
| unarchiveSession(sessionId: string): boolean { | |
| const trimmed = sessionId.trim(); | |
| if (!trimmed) return false; | |
| const existing = db.get<{ present: number }>( | |
| "select 1 as present from terminal_sessions where id = ? limit 1", | |
| [trimmed], | |
| ); | |
| if (!existing) return false; | |
| db.run("update terminal_sessions set archived_at = null where id = ?", [trimmed]); | |
| emitChanged({ sessionId: trimmed, reason: "meta-updated" }); | |
| return true; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/sessions/sessionService.ts` around lines 558 -
571, archiveSession and unarchiveSession currently always return true and call
emitChanged even when no terminal_sessions row exists; change them to verify the
update actually affected a row before emitting or returning success by checking
the database run result (e.g., the statement result's changes/count or use a
preceding SELECT to confirm existence). Specifically, update the implementations
referencing db.run("update terminal_sessions ...", ...) to inspect the result
(changes > 0) and only call emitChanged({ sessionId: trimmed, reason:
"meta-updated" }) and return true when a row was modified; otherwise return
false (and do not emit). Keep the same parameter handling and trimmed variable
names.
| case "K": | ||
| ensureCursor() | ||
| if first == 1 { | ||
| lines[row].removeFirst(min(column, lines[row].count)) | ||
| column = 0 | ||
| } else if first == 2 { |
There was a problem hiding this comment.
CSI 1K handling currently shifts text left and moves the cursor incorrectly.
removeFirst + column = 0 does not match erase-in-line semantics. It should clear the prefix in place and keep cursor position.
Suggested fix
case "K":
ensureCursor()
if first == 1 {
- lines[row].removeFirst(min(column, lines[row].count))
- column = 0
+ let eraseCount = min(column + 1, lines[row].count)
+ if eraseCount > 0 {
+ for idx in 0..<eraseCount {
+ lines[row][idx] = " "
+ }
+ }
} else if first == 2 {
lines[row].removeAll()
column = 0
} else if column < lines[row].count {
lines[row].removeSubrange(column..<lines[row].count)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swift` around
lines 199 - 204, The CSI "K" (case "K") branch incorrectly shifts text left by
calling lines[row].removeFirst(...) and setting column = 0; instead implement
erase-in-line semantics for parameter first == 1 by replacing the prefix up to
min(column, lines[row].count) with the same number of space characters in-place
and do not change the cursor position. Locate the case "K" block
(ensureCursor(), lines[row], column, first) and change the logic so it writes
spaces into the range 0..<min(column, lines[row].count) (preserving string
length) and remove the assignment column = 0.
CI: - Regenerate apps/ios/ADE/Resources/DatabaseBootstrap.sql so the iOS bootstrap artifact matches the new terminal_sessions.archived_at column (kvDb.bootstrapSql.test was failing on test-desktop shard 5). Review fixes: - iOS WorkRootScreen: archivedSessionIds now treats remote archivedAt as authoritative for sessions present in the remote payload; toggle paths operate on raw local storage so remote state does not get shadowed by stale local ids (greptile P1, coderabbit major). - iOS WorkNavigationAndTranscriptHelpers: CSI 1K now blanks columns in place with spaces and leaves the cursor column unchanged, per ECMA-48 (capy-ai, coderabbit). - AgentChatMessageList: jump-to-latest pill no longer requires an active turn; programmatic-scroll counter only increments when the scrollTop assignment actually moves the element (greptile P2 x2). - agentChatService: file-backed slash expansion also defers to any runtime-registered slash command of the same name (coderabbit). - claude/codexSlashCommandDiscovery: resolution falls back to a normalized-name directory walk so non-canonical filenames round trip; missing $ARGUMENTS templates now append trailing args instead of dropping them silently (coderabbit x3). - sessionService: archive/unarchive return false and skip meta-updated emission when the session id does not exist (coderabbit major). - AgentChatComposer: /clear is hidden from the slash menu when no onClearEvents handler is wired, and the menu closes once the user types a space after the command name so Enter submits with args (coderabbit x2). - packagedRuntimeSmoke: flush stdout via the write callback before process.exit(0) (coderabbit minor). - release.md: typo build5_present -> build_present (coderabbit minor). Deferred to next iteration if reviewers re-flag: - AgentChatPane window.confirm() in onContextMenu / delete handler -> warrants a useState-backed modal; non-trivial refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@copilot review but do not make fixes |
Here's a review of the changes since the last review. The follow-up commit ( Greptile P1 — iOS local storage contamination (FIXED)
This is a correct fix for the behavioral bug, though the non-chat write-back path still contaminates storage with extra IDs that are silently masked by the getter. Greptile P2 — Jump-to-latest pill (FIXED)
The inline comment still says "only meaningful during an active turn" but the condition no longer enforces that — this is a minor stale comment, not a bug. Greptile P2 — Scroll counter leak (FIXED)The One remaining edge case: iOS CSI 1K (EL erase-from-start) (FIXED)The original Minor nit:
|
ship-lane handoff: clear-to-mergeCI green (all 24 checks pass on Iterations
Intentionally not addressed (judgment calls)
Action requestedPer |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR delivers several chat UX improvements: reasoning cards no longer auto-expand during streaming (collapsed by default, user-opt-in), a new jump-to-latest pill surfaces whenever the user scrolls away from the bottom of a non-ended session, and the scroll engine is reworked (MutationObserver → ResizeObserver, rAF coalescing, programmatic-scroll counter) to eliminate flicker and unwanted snap-back. It also adds archive/unarchive for desktop chat sessions (context-menu + select dropdown), project-level slash commands for both Claude and Codex with
$ARGUMENTSsubstitution, a full VT100 screen-replay terminal sanitizer on iOS, and iOS chat session deletion.Confidence Score: 5/5
Safe to merge; all remaining findings are minor style/comment issues with no correctness impact.
The scroll rework is well-guarded (guards against stuck counter, zero-height elements, missing ResizeObserver). The dedup fix correctly handles the snapshot-vs-delta race. Archive/unarchive is idempotent (COALESCE) with a proper migration. Only a stale inline comment was found.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User (right-click tab) participant P as AgentChatPane participant D as ConfirmDialog participant IPC as IPC (archive/unarchive) participant S as sessionService (SQLite) U->>P: onContextMenu → requestArchiveChat P->>D: archiveConfirm.confirmAsync D-->>P: ok = true P->>IPC: window.ade.agentChat.archive({ sessionId }) IPC->>S: archiveSession (UPDATE SET archived_at = COALESCE(archived_at, now)) S-->>IPC: ok IPC-->>P: void P->>P: invalidateSessionListCache() P->>P: setSelectedSessionId(null) if active P->>IPC: agentChat.list({ laneId }) IPC-->>P: allRows P->>P: filter → rows (active) + archivedSessions P->>P: setSessions / setArchivedSessions note over U,S: Unarchive via Archived (N) dropdown U->>P: onChange (select archived session) P->>IPC: window.ade.agentChat.unarchive({ sessionId }) IPC->>S: unarchiveSession (UPDATE SET archived_at = NULL) S-->>IPC: ok IPC-->>P: void P->>P: refreshSessions → session reappears in tabsPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "ship: iter 1 — fix kvDb bootstrap drift ..." | Re-trigger Greptile