Cursor Chat Thinking Tool Grouping#727
Merged
Merged
Conversation
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR upgrades Node.js/@cursor/sdk requirements, fixes ade-cli model id handling and CLI-missing error classification, introduces a shared "activity phase" row-collapsing utility applied to ade-cli TUI, desktop chat transcript, and iOS work timeline rendering, and adds structured Cursor SDK error details plus agent-mode threading across desktop main-process services and iOS error display. ChangesCursor SDK Version Bump and ade-cli Fixes
Activity Phase Collapsing
Cursor SDK Structured Errors and Agent Mode
iOS Error Detail Propagation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
July 8, 2026 00:03
arul28
force-pushed
the
ade/start-skill-then-plan-using-260c3b15
branch
from
feaf2f5 to
289d593
Compare
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: 289d5936ae
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d1d12930
ℹ️ 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".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4405a7a46
ℹ️ 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".
| case .error(let message, let turnId, _, let errorInfo): | ||
| let detailText = prettyPrintedRemoteJSONValue(errorInfo) | ||
| case .error(let message, let detail, let turnId, _, let errorInfo): | ||
| let detailText = detail ?? prettyPrintedRemoteJSONValue(errorInfo) |
There was a problem hiding this comment.
When an error event includes both detail and errorInfo (the new Cursor SDK failures do), this now chooses detailText from detail and then recomputes the category only from message/detail. That drops the structured errorInfo.category, so cases like message: "Cursor SDK stream failed.", detail: "Cursor request ID: ...", and errorInfo: {category: "network"} render as a generic error instead of the network/rate-limit styling. Read the category from errorInfo first and only fall back to workErrorCategory when it is absent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/renderer/components/chat/AgentChatMessageList.tsx (1)
4251-4270: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
resolveAnchoredChatRowIndexneeds the same seam fallback as prepend anchoring.
A prefix regroup can keep the anchor row under its original key, while the fullgroupedRowslater collapses that phase into anactivity-phase-*key. The exactkeymatch then returns-1and can trigger unnecessary older-history retries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 4251 - 4270, resolveAnchoredChatRowIndex is only doing an exact row-key match, so it misses cases where a prefix regroup still keeps the anchor under its original key but the final groupedRows collapses that phase into an activity-phase-* seam key. Update the lookup in resolveAnchoredChatRowIndex to include the same seam fallback used by prepend anchoring: after the direct key match fails, resolve against the corresponding seam/activity-phase row instead of returning -1. Keep the change localized to resolveAnchoredChatRowIndex and reuse the existing grouping/key logic in AgentChatMessageList.tsx.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 3046-3055: The network-return branch in agentChatService is
matching Cursor internal errors too early, causing bare objects without a usable
top-level message to be stringified as “[object Object]”. Update the
error-handling flow around the classification logic in agentChatService so the
internal_error/internal error promotion happens before the network/transport
early return, or explicitly bypass that return when the message is not usable.
Keep the fix localized to the existing cursorSdk handling and the combinedLower
checks in that method.
In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts`:
- Around line 399-425: Preserve the network category for worker-emitted errors
in cursorSdkEventMapper by ensuring the error classification does not drop
adeErrorCode values of "network" when classifyCursorSdkErrorText(...) returns
unknown. Update the errorInfo selection in the error event assembly to fall back
to errorCode === "network", so the emitted event keeps the network category even
when detail text lacks a transport token.
In `@apps/desktop/src/shared/chatActivityPhase.test.ts`:
- Around line 68-84: The test currently only verifies breaking on assistant
text, not on turn changes, so it does not cover the turn-boundary branch in
collapseActivityPhaseRows. Update the chatActivityPhase test by adding a
separate case with consecutive activity rows using different turnId values to
exercise the `(nextActivity.turnId ?? null) !== (phaseTurnId ?? null)` path and
assert the phase is split correctly, while keeping the existing text-boundary
coverage intact.
In `@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift`:
- Around line 1317-1397: The three merge helpers in WorkTimelineHelpers.swift
are using invalid force unwraps on array subscripts, which prevents compilation.
Update mergeReasoningTimelineEntries, mergeToolGroupTimelineEntries, and
mergeChangedFilesTimelineEntries to use plain array access for the first/last
WorkTimelineEntry values instead of entries[0]! or similar forced unwrap syntax,
while keeping the existing first/last entry logic intact.
- Around line 1282-1307: Thread the turn id through work-phase cards before
phase collapsing, since activityPhaseTurnId(for:) only reads reasoning event
cards and activityPhaseMemberKind(for:) treats .toolGroup and .changedFiles as
work without any turn metadata. Update the work card models (WorkToolCardModel,
WorkCommandCardModel, WorkFileChangeCardModel) and the related timeline assembly
so each work card carries the originating turnId, then use that in the phase
boundary logic to prevent work entries from being merged across turns. Keep the
fix localized around activityPhaseTurnId(for:) and
activityPhaseMemberKind(for:), ensuring the turn-based grouping can distinguish
adjacent work clusters correctly.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 4251-4270: resolveAnchoredChatRowIndex is only doing an exact
row-key match, so it misses cases where a prefix regroup still keeps the anchor
under its original key but the final groupedRows collapses that phase into an
activity-phase-* seam key. Update the lookup in resolveAnchoredChatRowIndex to
include the same seam fallback used by prepend anchoring: after the direct key
match fails, resolve against the corresponding seam/activity-phase row instead
of returning -1. Keep the change localized to resolveAnchoredChatRowIndex and
reuse the existing grouping/key logic in AgentChatMessageList.tsx.
🪄 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: 57f9e33f-11b9-40ab-bb12-3dfff56f33b6
⛔ Files ignored due to path filters (6)
README.mdis excluded by!*.mdapps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonapps/desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**
📒 Files selected for processing (30)
apps/ade-cli/README.mdapps/ade-cli/package.jsonapps/ade-cli/src/cursorCloud.test.tsapps/ade-cli/src/cursorCloud.tsapps/ade-cli/src/services/agentRegistry.test.tsapps/ade-cli/src/services/agentRegistry.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/aggregate.test.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/desktop/package.jsonapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorSdkErrors.test.tsapps/desktop/src/main/services/chat/cursorSdkErrors.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.test.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.tsapps/desktop/src/main/services/chat/cursorSdkPool.test.tsapps/desktop/src/main/services/chat/cursorSdkPool.tsapps/desktop/src/main/services/chat/cursorSdkProtocol.tsapps/desktop/src/main/services/chat/cursorSdkWorker.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/shared/chatActivityPhase.test.tsapps/desktop/src/shared/chatActivityPhase.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swift
Comment on lines
3046
to
+3055
| if ( | ||
| combinedLower.includes("timeout") | ||
| cursorClassification.kind === "network" | ||
| || errorCode === "network" | ||
| || errorCode === "transport" | ||
| || combinedLower.includes("nghttp2") | ||
| || combinedLower.includes("http/2") | ||
| || combinedLower.includes("stream closed with error") | ||
| || (options.cursorSdk && combinedLower.includes("internal_error")) | ||
| || (options.cursorSdk && combinedLower.includes("internal error")) | ||
| || combinedLower.includes("timeout") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section around the cited lines
sed -n '3010,3105p' apps/desktop/src/main/services/chat/agentChatService.ts
echo '---'
# Find readErrorMessage and related promotion logic in the same file
rg -n "readErrorMessage|object Object|internal-error|internal error|structuredCursorDetail|cursorSdk" apps/desktop/src/main/services/chat/agentChatService.tsRepository: arul28/ADE
Length of output: 8430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2798,2878p' apps/desktop/src/main/services/chat/agentChatService.ts
echo '---'
sed -n '2878,2990p' apps/desktop/src/main/services/chat/agentChatService.tsRepository: arul28/ADE
Length of output: 7843
Cursor internal errors still surface as [object Object]
options.cursorSdk now sends internal_error / internal error through the network return first, so a bare object with no top-level message still becomes "[object Object]" and never reaches the later promotion branch. Move the promotion ahead of this return, or skip the network path when the message isn’t usable.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 3046 -
3055, The network-return branch in agentChatService is matching Cursor internal
errors too early, causing bare objects without a usable top-level message to be
stringified as “[object Object]”. Update the error-handling flow around the
classification logic in agentChatService so the internal_error/internal error
promotion happens before the network/transport early return, or explicitly
bypass that return when the message is not usable. Keep the fix localized to the
existing cursorSdk handling and the combinedLower checks in that method.
Comment on lines
+399
to
+425
| // The streamed ERROR status often carries no reason; the worker injects | ||
| // the run result/store detail after the run settles. | ||
| const errorCode = readString(record.adeErrorCode); | ||
| const message = errorCode | ||
| ? `Cursor run failed: ${errorCode}` | ||
| : detail ?? "Cursor SDK run failed."; | ||
| const transport = isCursorSdkTransportErrorText(errorCode ?? detail); | ||
| const errorDetail = asRecord(record.adeErrorDetail); | ||
| const detailMessage = readString(errorDetail?.message); | ||
| const detailCode = readString(errorDetail?.code); | ||
| const detailName = readString(errorDetail?.name); | ||
| const requestId = readString(errorDetail?.requestId); | ||
| const classification = classifyCursorSdkErrorText(errorCode, detail, detailMessage, detailCode, detailName); | ||
| const message = cursorSdkErrorMessage(classification.kind, errorCode, detail); | ||
| const eventDetail = uniqueLines([ | ||
| detailMessage && detailMessage !== message ? detailMessage : null, | ||
| detail && detail !== message && detail !== detailMessage ? detail : null, | ||
| detailCode && detailCode !== errorCode ? `Code: ${detailCode}` : null, | ||
| requestId ? `Cursor request ID: ${requestId}` : null, | ||
| ]); | ||
| const errorInfo = errorEventInfo(classification.kind); | ||
| return [ | ||
| ...compactionEvents, | ||
| { | ||
| type: "error" as const, | ||
| message, | ||
| turnId, | ||
| ...(transport ? { errorInfo: { category: "network" as const } } : {}), | ||
| }]; | ||
| type: "error" as const, | ||
| message, | ||
| turnId, | ||
| ...(eventDetail ? { detail: eventDetail } : {}), | ||
| ...(errorInfo ? { errorInfo } : {}), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm transport classifier does not match the literal "network"/"transport" strings.
rg -nP -C3 'function (isCursorSdkTransportErrorText|classifyCursorSdkErrorText)' apps/desktop/src/main/services/chat/cursorSdkProtocol.ts
# Confirm worker emits errorCode "network"/"transport"
rg -nP 'errorCode:\s*"(network|transport)"' apps/desktop/src/main/services/chat/cursorSdkWorker.tsRepository: arul28/ADE
Length of output: 788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cursorSdkProtocol.ts: transport classifier + error classification ---'
sed -n '288,360p' apps/desktop/src/main/services/chat/cursorSdkProtocol.ts
echo
echo '--- cursorSdkEventMapper.ts: errorEventInfo + ERROR block context ---'
sed -n '150,220p' apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts
sed -n '399,425p' apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts
echo
echo '--- cursorSdkWorker.ts: errorCode emissions context ---'
sed -n '140,180p' apps/desktop/src/main/services/chat/cursorSdkWorker.tsRepository: arul28/ADE
Length of output: 8096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "adeErrorCode|adeErrorDetail|errorInfo\\(|classifyWorkerError|errorCode: \"network\"|errorCode: \"transport\"" apps/desktop/src/main/services/chatRepository: arul28/ADE
Length of output: 2440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,290p' apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.tsRepository: arul28/ADE
Length of output: 3136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '"network"|errorInfo: \{ category: "network" \}' apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts apps/desktop/src/main/services/chat/cursorSdkWorker.tsRepository: arul28/ADE
Length of output: 839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- event mapper tests with literal "network" ---'
rg -n '"network"' apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts
echo
echo '--- worker classify/output around network path ---'
sed -n '120,175p' apps/desktop/src/main/services/chat/cursorSdkWorker.ts
sed -n '520,565p' apps/desktop/src/main/services/chat/cursorSdkWorker.ts
sed -n '788,810p' apps/desktop/src/main/services/chat/cursorSdkWorker.tsRepository: arul28/ADE
Length of output: 4527
Preserve the network category for worker-emitted errors. errorInfo comes only from classifyCursorSdkErrorText(...), so a worker adeErrorCode: "network" can fall through to unknown and lose { category: "network" } when the detail text doesn’t include a transport token. Fall back to errorCode === "network" here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts` around lines 399
- 425, Preserve the network category for worker-emitted errors in
cursorSdkEventMapper by ensuring the error classification does not drop
adeErrorCode values of "network" when classifyCursorSdkErrorText(...) returns
unknown. Update the errorInfo selection in the error event assembly to fall back
to errorCode === "network", so the emitted event keeps the network category even
when detail text lacks a transport token.
Comment on lines
+68
to
+84
| it("breaks phases on assistant text and turn boundaries", () => { | ||
| const rows: TestRow[] = [ | ||
| { id: "r1", kind: "reasoning", turnId: "turn-1", text: "A" }, | ||
| { id: "w1", kind: "work", turnId: "turn-1" }, | ||
| { id: "t1", kind: "text", turnId: "turn-1", text: "Answer" }, | ||
| { id: "r2", kind: "reasoning", turnId: "turn-1", text: "B" }, | ||
| { id: "w2", kind: "work", turnId: "turn-1" }, | ||
| { id: "r3", kind: "reasoning", turnId: "turn-1", text: "C" }, | ||
| { id: "w3", kind: "work", turnId: "turn-1" }, | ||
| ]; | ||
| const collapsed = collapseActivityPhaseRows( | ||
| rows, | ||
| (row) => (row.kind === "text" ? null : { kind: row.kind === "work" ? "work" : "reasoning", turnId: row.turnId }), | ||
| (): TestRow[] => [{ id: "merged", kind: "reasoning", turnId: "turn-1" }], | ||
| ); | ||
| expect(collapsed.map((row) => row.id)).toEqual(["r1", "w1", "t1", "merged"]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test title claims "turn boundaries" but all rows share turnId: "turn-1".
Only assistant-text breaking is exercised — the collapseActivityPhaseRows turnId-change break path ((nextActivity.turnId ?? null) !== (phaseTurnId ?? null)) is not covered. Add a case where consecutive activity rows have different turnIds to verify the phase breaks correctly.
🧪 Suggested additional test case
+ it("breaks phases on turn boundaries", () => {
+ const rows: TestRow[] = [
+ { id: "r1", kind: "reasoning", turnId: "turn-1", text: "A" },
+ { id: "w1", kind: "work", turnId: "turn-1" },
+ { id: "r2", kind: "reasoning", turnId: "turn-2", text: "B" },
+ { id: "w2", kind: "work", turnId: "turn-2" },
+ ];
+ const collapsed = collapseActivityPhaseRows(
+ rows,
+ (row) => (row.kind === "text" ? null : { kind: row.kind === "work" ? "work" : "reasoning", turnId: row.turnId }),
+ (): TestRow[] => [{ id: "merged", kind: "reasoning", turnId: "turn-1" }],
+ );
+ // Two phases of 2 rows each — neither meets the collapse threshold.
+ expect(collapsed.map((row) => row.id)).toEqual(["r1", "w1", "r2", "w2"]);
+ });📝 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.
Suggested change
| it("breaks phases on assistant text and turn boundaries", () => { | |
| const rows: TestRow[] = [ | |
| { id: "r1", kind: "reasoning", turnId: "turn-1", text: "A" }, | |
| { id: "w1", kind: "work", turnId: "turn-1" }, | |
| { id: "t1", kind: "text", turnId: "turn-1", text: "Answer" }, | |
| { id: "r2", kind: "reasoning", turnId: "turn-1", text: "B" }, | |
| { id: "w2", kind: "work", turnId: "turn-1" }, | |
| { id: "r3", kind: "reasoning", turnId: "turn-1", text: "C" }, | |
| { id: "w3", kind: "work", turnId: "turn-1" }, | |
| ]; | |
| const collapsed = collapseActivityPhaseRows( | |
| rows, | |
| (row) => (row.kind === "text" ? null : { kind: row.kind === "work" ? "work" : "reasoning", turnId: row.turnId }), | |
| (): TestRow[] => [{ id: "merged", kind: "reasoning", turnId: "turn-1" }], | |
| ); | |
| expect(collapsed.map((row) => row.id)).toEqual(["r1", "w1", "t1", "merged"]); | |
| }); | |
| it("breaks phases on assistant text and turn boundaries", () => { | |
| const rows: TestRow[] = [ | |
| { id: "r1", kind: "reasoning", turnId: "turn-1", text: "A" }, | |
| { id: "w1", kind: "work", turnId: "turn-1" }, | |
| { id: "t1", kind: "text", turnId: "turn-1", text: "Answer" }, | |
| { id: "r2", kind: "reasoning", turnId: "turn-1", text: "B" }, | |
| { id: "w2", kind: "work", turnId: "turn-1" }, | |
| { id: "r3", kind: "reasoning", turnId: "turn-1", text: "C" }, | |
| { id: "w3", kind: "work", turnId: "turn-1" }, | |
| ]; | |
| const collapsed = collapseActivityPhaseRows( | |
| rows, | |
| (row) => (row.kind === "text" ? null : { kind: row.kind === "work" ? "work" : "reasoning", turnId: row.turnId }), | |
| (): TestRow[] => [{ id: "merged", kind: "reasoning", turnId: "turn-1" }], | |
| ); | |
| expect(collapsed.map((row) => row.id)).toEqual(["r1", "w1", "t1", "merged"]); | |
| }); | |
| it("breaks phases on turn boundaries", () => { | |
| const rows: TestRow[] = [ | |
| { id: "r1", kind: "reasoning", turnId: "turn-1", text: "A" }, | |
| { id: "w1", kind: "work", turnId: "turn-1" }, | |
| { id: "r2", kind: "reasoning", turnId: "turn-2", text: "B" }, | |
| { id: "w2", kind: "work", turnId: "turn-2" }, | |
| ]; | |
| const collapsed = collapseActivityPhaseRows( | |
| rows, | |
| (row) => (row.kind === "text" ? null : { kind: row.kind === "work" ? "work" : "reasoning", turnId: row.turnId }), | |
| (): TestRow[] => [{ id: "merged", kind: "reasoning", turnId: "turn-1" }], | |
| ); | |
| // Two phases of 2 rows each — neither meets the collapse threshold. | |
| expect(collapsed.map((row) => row.id)).toEqual(["r1", "w1", "r2", "w2"]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/shared/chatActivityPhase.test.ts` around lines 68 - 84, The
test currently only verifies breaking on assistant text, not on turn changes, so
it does not cover the turn-boundary branch in collapseActivityPhaseRows. Update
the chatActivityPhase test by adding a separate case with consecutive activity
rows using different turnId values to exercise the `(nextActivity.turnId ??
null) !== (phaseTurnId ?? null)` path and assert the phase is split correctly,
while keeping the existing text-boundary coverage intact.
Comment on lines
+1282
to
+1307
| private func activityPhaseTurnId(for entry: WorkTimelineEntry) -> String? { | ||
| if case .eventCard(let card) = entry.payload, card.kind == "reasoning" { | ||
| let parts = card.id.split(separator: ":", omittingEmptySubsequences: false).map(String.init) | ||
| if let turnIndex = parts.firstIndex(of: "turn"), turnIndex + 1 < parts.count { | ||
| let candidate = parts[turnIndex + 1].trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !candidate.isEmpty { return candidate } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| private enum ActivityPhaseMemberKind { | ||
| case reasoning | ||
| case work | ||
| } | ||
|
|
||
| private func activityPhaseMemberKind(for entry: WorkTimelineEntry) -> ActivityPhaseMemberKind? { | ||
| switch entry.payload { | ||
| case .eventCard(let card) where card.kind == "reasoning": | ||
| return .reasoning | ||
| case .toolGroup, .changedFiles: | ||
| return .work | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether WorkToolGroupMember / WorkToolCardModel / WorkCommandCardModel /
# WorkFileChangeCardModel carry a turnId that could be used to extend
# activityPhaseTurnId to work entries, mirroring the desktop implementation.
rg -n "struct WorkToolCardModel|struct WorkCommandCardModel|struct WorkFileChangeCardModel|struct WorkToolGroupModel|enum WorkToolGroupMember" -A 20 apps/ios/ADE/Views/Work/WorkEventMapping.swift apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift 2>/dev/null | rg -n "turnId" -B5Repository: arul28/ADE
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map Swift files under the iOS work views area.
fd -e swift . apps/ios/ADE/Views/Work | sort
# Locate likely model and helper symbols related to work timeline rendering.
rg -n "activityPhaseTurnId|activityPhaseMemberKind|collapseActivityPhaseTimelineEntries|buildWorkTimeline|turnId|WorkToolGroup|WorkToolCard|WorkCommandCard|WorkFileChangeCard|WorkTimelineEntry" apps/ios/ADE/Views/Work -g '*.swift'
# Show outlines for the most relevant helpers file(s).
ast-grep outline apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift --view expandedRepository: arul28/ADE
Length of output: 50366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -e swift . apps/ios/ADE/Views/Work | sort
rg -n "activityPhaseTurnId|activityPhaseMemberKind|collapseActivityPhaseTimelineEntries|buildWorkTimeline|turnId|WorkToolGroup|WorkToolCard|WorkCommandCard|WorkFileChangeCard|WorkTimelineEntry" apps/ios/ADE/Views/Work -g '*.swift'
ast-grep outline apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift --view expandedRepository: arul28/ADE
Length of output: 50366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the collapse logic around the cited lines.
sed -n '1270,1455p' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift | cat -n
printf '\n==== MODEL DEFINITIONS ====\n'
# Inspect the work card/group models and any turnId fields.
rg -n "struct WorkToolGroupModel|struct WorkToolGroupMember|struct WorkToolCardModel|struct WorkCommandCardModel|struct WorkFileChangeCardModel|enum WorkTimelineEntryPayload" apps/ios/ADE/Views/Work -A 30 -g '*.swift'
printf '\n==== BUILD PIPELINE SLICE ====\n'
sed -n '923,1060p' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift | cat -nRepository: arul28/ADE
Length of output: 25371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the work-group member model and the card builders that feed the timeline.
sed -n '1,180p' apps/ios/ADE/Views/Work/WorkModels.swift | cat -n
printf '\n==== GROUP MEMBER AREA ====\n'
sed -n '470,560p' apps/ios/ADE/Views/Work/WorkModels.swift | cat -n
printf '\n==== TOOL/FILE BUILDERS ====\n'
sed -n '1480,1535p' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift | cat -n
printf '\n==== WORK TOOL GROUP MEMBER EXTRACTION ====\n'
sed -n '1148,1215p' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift | cat -nRepository: arul28/ADE
Length of output: 15910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the remainder of collapseActivityPhaseTimelineEntries and nearby helpers.
sed -n '1401,1485p' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift | cat -n
printf '\n==== DESKTOP COMPARISON ====\n'
rg -n "classifyActivityPhaseRow|collapseGroupedActivityPhaseRows|work_log_group|turnId" apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts -A 80 -g '*.ts'Repository: arul28/ADE
Length of output: 42410
Thread turnId through work cards before collapsing activity phases. activityPhaseTurnId only ever returns a value for reasoning cards, while WorkToolCardModel, WorkCommandCardModel, and WorkFileChangeCardModel drop the turn id entirely. That leaves the phase boundary check unable to stop a work cluster from being merged under the wrong turn unless a hard boundary happens to sit between them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift` around lines 1282 - 1307,
Thread the turn id through work-phase cards before phase collapsing, since
activityPhaseTurnId(for:) only reads reasoning event cards and
activityPhaseMemberKind(for:) treats .toolGroup and .changedFiles as work
without any turn metadata. Update the work card models (WorkToolCardModel,
WorkCommandCardModel, WorkFileChangeCardModel) and the related timeline assembly
so each work card carries the originating turnId, then use that in the phase
boundary logic to prevent work entries from being merged across turns. Keep the
fix localized around activityPhaseTurnId(for:) and
activityPhaseMemberKind(for:), ensuring the turn-based grouping can distinguish
adjacent work clusters correctly.
Source: Path instructions
Comment on lines
+1317
to
+1397
| private func mergeReasoningTimelineEntries(_ entries: [WorkTimelineEntry]) -> WorkTimelineEntry { | ||
| let cards = entries.compactMap { entry -> WorkEventCardModel? in | ||
| guard case .eventCard(let card) = entry.payload, card.kind == "reasoning" else { return nil } | ||
| return card | ||
| } | ||
| let first = entries[0]! | ||
| let last = entries[entries.count - 1]! | ||
| let mergedBody = cards | ||
| .compactMap { $0.body?.trimmingCharacters(in: .whitespacesAndNewlines) } | ||
| .filter { !$0.isEmpty } | ||
| .joined(separator: "\n\n---\n\n") | ||
| let anchor = cards.first ?? WorkEventCardModel( | ||
| id: first.id, | ||
| kind: "reasoning", | ||
| title: "Reasoning", | ||
| icon: "brain.head.profile", | ||
| tint: .secondary, | ||
| timestamp: first.timestamp, | ||
| body: nil, | ||
| bullets: [], | ||
| metadata: [] | ||
| ) | ||
| let mergedCard = WorkEventCardModel( | ||
| id: "activity-phase-reasoning:\(first.id)", | ||
| kind: anchor.kind, | ||
| title: anchor.title, | ||
| icon: anchor.icon, | ||
| tint: anchor.tint, | ||
| timestamp: last.timestamp, | ||
| body: mergedBody.isEmpty ? anchor.body : mergedBody, | ||
| bullets: anchor.bullets, | ||
| metadata: anchor.metadata, | ||
| planSteps: anchor.planSteps, | ||
| isInProgress: cards.contains(where: \.isInProgress), | ||
| questionModel: anchor.questionModel, | ||
| planApprovalModel: anchor.planApprovalModel, | ||
| resolution: anchor.resolution | ||
| ) | ||
| return WorkTimelineEntry( | ||
| id: mergedCard.id, | ||
| timestamp: last.timestamp, | ||
| rank: first.rank, | ||
| payload: .eventCard(mergedCard) | ||
| ) | ||
| } | ||
|
|
||
| private func mergeToolGroupTimelineEntries(_ entries: [WorkTimelineEntry]) -> WorkTimelineEntry { | ||
| let groups = entries.compactMap { entry -> WorkToolGroupModel? in | ||
| guard case .toolGroup(let group) = entry.payload else { return nil } | ||
| return group | ||
| } | ||
| let first = entries[0]! | ||
| let merged = WorkToolGroupModel( | ||
| id: "activity-phase-tools:\(first.id)", | ||
| members: groups.flatMap(\.members) | ||
| ) | ||
| return WorkTimelineEntry( | ||
| id: merged.id, | ||
| timestamp: entries[entries.count - 1].timestamp, | ||
| rank: first.rank, | ||
| payload: .toolGroup(merged) | ||
| ) | ||
| } | ||
|
|
||
| private func mergeChangedFilesTimelineEntries(_ entries: [WorkTimelineEntry]) -> WorkTimelineEntry { | ||
| let groups = entries.compactMap { entry -> WorkChangedFilesGroupModel? in | ||
| guard case .changedFiles(let group) = entry.payload else { return nil } | ||
| return group | ||
| } | ||
| let first = entries[0]! | ||
| let merged = WorkChangedFilesGroupModel( | ||
| id: "activity-phase-files:\(first.id)", | ||
| files: groups.flatMap(\.files) | ||
| ) | ||
| return WorkTimelineEntry( | ||
| id: merged.id, | ||
| timestamp: entries[entries.count - 1].timestamp, | ||
| rank: first.rank, | ||
| payload: .changedFiles(merged) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm all occurrences of the invalid force-unwrap pattern in this file.
rg -n 'entries\[0\]!|entries\[entries\.count - 1\]!' apps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftRepository: arul28/ADE
Length of output: 287
Remove the stray force unwraps in WorkTimelineHelpers.swift
entries[0]! is invalid Swift for Array<Element> and stops this file from compiling. Drop the ! in the three merge helpers around lines 1322, 1323, 1368, and 1386.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift` around lines 1317 - 1397,
The three merge helpers in WorkTimelineHelpers.swift are using invalid force
unwraps on array subscripts, which prevents compilation. Update
mergeReasoningTimelineEntries, mergeToolGroupTimelineEntries, and
mergeChangedFilesTimelineEntries to use plain array access for the first/last
WorkTimelineEntry values instead of entries[0]! or similar forced unwrap syntax,
while keeping the existing first/last entry logic intact.
Source: Path instructions
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
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR updates the Cursor chat integration and transcript handling. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
No files need follow-up attention.
What T-Rex did
Important Files Changed
Reviews (3): Last reviewed commit: "Fix Cursor SDK resume recovery" | Re-trigger Greptile