Preserve Claude chat resume state and surface orphan lanes#175
Merged
Conversation
Keep Claude runtime resume metadata (sdkSessionId, lane directive, runtimeInvalidated) intact when a managed chat is torn down for a non-terminal reason (idle_ttl, budget_eviction, pool_compaction, paused_run, project_close, shutdown). Only terminal reasons (handle_close, ended_session, model_switch) fully invalidate the runtime now, so subsequent turns can re-attach to the same V2 session instead of starting cold. Render orphan-lane sessions (whose laneId is missing from the current lanes list) as their own collapsible sticky groups in the Work session list on desktop and iOS. Sort by latest startedAt with name tiebreak and label with the session's preserved laneName. Grow the iOS chat history window: request a 2 MB snapshot, merge truncated snapshots with existing events instead of replacing, retain up to 1,000 events per session, and fall back to messageId when assistant text is missing itemId so Claude- and Codex-produced turns stay aligned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds agent chat event-history APIs and IPC, per-session in-memory history with lazy hydration, Claude resume-state persistence on selective teardowns, runtime-controllable sync host discovery and configurable/shared local device ID handling, UI changes for snapshot-first chat history hydration and orphan-lane session grouping, plus related iOS sync/chat and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Fix shutdown resume-state persistence: move `managed.deleted = true` after
`teardownRuntime("shutdown")` in `forceDisposeAll` so its `persistChatState`
call actually writes the Claude sdkSessionId/lane directive before the
tombstone gate bails (capy-ai).
- Flip `WorkEventMapping.stableItemId` priority so `itemId` wins over
`messageId`, matching `WorkTranscriptParser` — same Claude turn now groups
identically live and on replay (Greptile P1).
- Normalize `itemId` via `optionalString` in `WorkTranscriptParser` text
branch so empty `itemId` falls through to `messageId` (capy-ai).
- Guard `latestStartedAt` in `SessionListPane` against `NaN` from malformed
`startedAt` values to keep orphan-lane sort deterministic (Greptile P2).
- Document `previousEnvelopeWasAssistantText` reset invariant at the flag's
declaration (Greptile P2).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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: 4048884885
ℹ️ 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".
…sync ordering Chat history and freeze recovery on remount - Add in-memory per-session chat event ring buffer in agentChatService alongside the on-disk transcript, wired into every commitChatEvent path. - New getChatEventHistory() merges the buffer with the on-disk transcript on first read, so a renderer that missed live events (project switch, tab switch, transient IPC drop) still recovers the full timeline. - Renderer AgentChatPane prefers the new snapshot API and clears its loaded-history gate in the catch path so a transient read failure no longer leaves the pane frozen with no retry. Single-session (Work tile) now force-reloads on mount, matching the other chat surfaces. - Clean up the buffer on deleteSession so a re-created session id doesn't inherit stale events. teardownRuntime no longer clobbers preserved Claude resume metadata - If teardownRuntime runs a second time on a session whose runtime was already torn down (e.g., idle_ttl eviction followed by app shutdown), bail early rather than falling through to the invalidating path that would reset runtimeInvalidated and clearLaneDirectiveKey. - Addresses PR #175 review comment on agentChatService.ts:5526. - Regression test: Claude session survives idle_ttl → shutdown with sdkSessionId and lastLaneDirectiveKey intact. iOS chat event ordering across truncated/merged snapshots - deduplicatedChatEventHistory now parses ISO-8601 timestamps with a fractional-seconds formatter + no-fraction fallback before comparing, so mixed-precision snapshots from the desktop sort chronologically instead of by lexical byte order. Addresses PR #175 review on SyncService.swift:4533. - recordChatEventEnvelope falls back to the full dedup/sort path when a live envelope predates the last-recorded envelope — keeps the common append-in-order fast path but heals out-of-order arrivals after a merge. Addresses PR #175 review on SyncService.swift:4504. - Regression tests for mixed fractional variants and delayed out-of-order live inserts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Pushed commit Review comments:
Bonus: chat-freeze bug. While verifying the review fixes, we discovered the desktop renderer had no snapshot-on-resubscribe path for chat events — switching projects or tabbing away could drop IPC events, and on return the pane saw only the transcript tail, missing in-flight events and (if |
![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 (4)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
1472-1491:⚠️ Potential issue | 🟠 MajorSort and de-dupe the realtime tail before appending.
The merge keeps
existingtail order as-is. If live IPC events arrive out of order or duplicate, the UI can render turns in the wrong order andderiveRuntimeState()can compute state from a non-canonical event sequence.Proposed fix
const tail = existing.filter((entry) => { if (lastParsedSequence != null && typeof entry.sequence === "number") { return entry.sequence > lastParsedSequence; } return entry.timestamp > lastParsedTs; }); - merged = tail.length ? [...parsed, ...tail] : parsed; + const seenTail = new Set<string>(); + const orderedTail = [...tail] + .sort((left, right) => { + if (typeof left.sequence === "number" && typeof right.sequence === "number" && left.sequence !== right.sequence) { + return left.sequence - right.sequence; + } + return left.timestamp.localeCompare(right.timestamp); + }) + .filter((entry) => { + const key = typeof entry.sequence === "number" + ? `seq:${entry.sequence}` + : `${entry.timestamp}:${entry.event.type}`; + if (seenTail.has(key)) return false; + seenTail.add(key); + return true; + }); + merged = orderedTail.length ? [...parsed, ...orderedTail] : parsed;🤖 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 1472 - 1491, The merge logic in AgentChatPane that builds merged from parsed and the real-time tail (using eventsBySessionRef.current, existing, parsed, tail, merged) must sort and deduplicate the tail before appending so out-of-order or duplicate IPC events don't corrupt the event sequence used by deriveRuntimeState(); update the tail construction to (1) sort by monotonic sequence when present (falling back to timestamp) and (2) dedupe events (using a stable unique key such as sequence if available, otherwise timestamp+sender/id) so merged becomes [...parsed, ...sortedDedupedTail] (or parsed if tail empty). Ensure you still prefer sequence ordering when available and keep sessionId/AgentChatEventEnvelope semantics intact.apps/desktop/src/main/services/sync/syncHostService.ts (1)
914-930:⚠️ Potential issue | 🟠 MajorSerialize tailnet unpublish before later publish attempts.
The new runtime toggle can leave tailnet discovery advertised after it is disabled. If a
tailscale serve ... targetpublish is already in flight,setDiscoveryEnabled(false)startsserve off, but the earlier publish process can complete after that and re-enable the service externally while its stale promise is ignored. A quick disable→enable has the same ordering hazard.Consider tracking the unpublish promise and only republishing after it settles, or forcing a second
serve offwhen a stale publish completes whilediscoveryEnabled === false.Also applies to: 1025-1047, 2056-2078
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 914 - 930, publishTailnetDiscovery can race with in-flight unpublish calls causing stale publishes to re-enable discovery; fix by serializing unpublish/publish: introduce and track a shared Promise (e.g., activeUnpublishPromise) that unpublishTailnetDiscovery assigns to, have publishTailnetDiscovery await that promise (or check its settled state) before issuing a new publish, and when a publish completes check discoveryEnabled and, if false, immediately trigger or await a fresh unpublishTailnetDiscovery to force-off any stale external state; apply the same pattern to the other publish/unpublish sites referenced (lines ~1025-1047 and ~2056-2078) to ensure all serve off/publish operations are serialized.apps/ios/ADE/Services/SyncService.swift (1)
4509-4562:⚠️ Potential issue | 🟠 MajorRespect
sequencewhen timestamps represent the same instant.The fast path appends equal-date events even when the incoming sequence is lower than the last event, and the full sort falls back to lexicographic timestamp when parsed dates are equal but string formats differ. That can misorder same-timestamp chat fragments.
Proposed fix
let canAppendInOrder: Bool = { guard let last = events.last else { return true } let lastDate = Self.parseIso8601(last.timestamp) let envelopeDate = Self.parseIso8601(envelope.timestamp) - if let lhs = envelopeDate, let rhs = lastDate { return lhs >= rhs } - return envelope.timestamp >= last.timestamp + if let lhs = envelopeDate, let rhs = lastDate { + if lhs != rhs { return lhs > rhs } + return (envelope.sequence ?? 0) >= (last.sequence ?? 0) + } + if envelope.timestamp != last.timestamp { + return envelope.timestamp > last.timestamp + } + return (envelope.sequence ?? 0) >= (last.sequence ?? 0) }()let lhsDate = Self.parseIso8601(lhs.timestamp) let rhsDate = Self.parseIso8601(rhs.timestamp) if lhsDate == rhsDate { - if lhs.timestamp == rhs.timestamp { - return (lhs.sequence ?? 0) < (rhs.sequence ?? 0) - } - return lhs.timestamp < rhs.timestamp + let lhsSequence = lhs.sequence ?? 0 + let rhsSequence = rhs.sequence ?? 0 + if lhsSequence != rhsSequence { return lhsSequence < rhsSequence } + return lhs.timestamp < rhs.timestamp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 4509 - 4562, The append-fast-path and the full dedup/sort path both fail to respect the envelope.sequence for events that represent the same instant; update the canAppendInOrder check and the comparator inside deduplicatedChatEventHistory to treat equal parsed timestamps as ordered by (sequence ?? 0) before falling back to string timestamps. Concretely: in the canAppendInOrder closure (the code comparing last.timestamp and envelope.timestamp using Self.parseIso8601) when envelopeDate == lastDate use (envelope.sequence ?? 0) >= (last.sequence ?? 0) to decide appendability; and inside deduplicatedChatEventHistory's sort comparator (where lhsDate == rhsDate) first compare (lhs.sequence ?? 0) vs (rhs.sequence ?? 0) and only if equal fall back to comparing timestamp strings.apps/ios/ADETests/ADETests.swift (1)
337-344:⚠️ Potential issue | 🟡 MinorAssert
maxBytesafter disconnect too.Line 339 only proves the initial subscription payload includes
maxBytes; a disconnect path could still drop it and this test would pass.🧪 Proposed test tightening
service.disconnect(clearCredentials: false) XCTAssertEqual(service.subscribedChatSessionIds, Set(["session-1", "session-2"])) - XCTAssertEqual(service.chatSubscriptionPayloads().compactMap { $0["sessionId"] as? String }.sorted(), ["session-1", "session-2"]) + let payloadsAfterDisconnect = service.chatSubscriptionPayloads() + XCTAssertEqual(payloadsAfterDisconnect.compactMap { $0["sessionId"] as? String }.sorted(), ["session-1", "session-2"]) + XCTAssertEqual(payloadsAfterDisconnect.compactMap { $0["maxBytes"] as? Int }, [2_000_000, 2_000_000])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADETests/ADETests.swift` around lines 337 - 344, The test currently asserts maxBytes only before disconnect; ensure the disconnect path preserves maxBytes by adding assertions after calling service.disconnect(clearCredentials: false) that validate service.chatSubscriptionPayloads().compactMap { $0["maxBytes"] as? Int } still equals [2_000_000, 2_000_000] (and keep the existing checks on subscribedChatSessionIds and sessionId payloads) so the subscription payloads retain their maxBytes through the disconnect flow in ADETests.swift.
♻️ Duplicate comments (1)
apps/desktop/src/main/services/sync/deviceRegistryService.ts (1)
146-158:⚠️ Potential issue | 🟠 MajorMake shared device ID creation and migration conflict-safe.
Two project registries initialized concurrently with the same missing
localDeviceIdPathcan both generate different UUIDs and race throughwriteTextAtomic; the last write wins on disk while the losing registry keeps a stale in-memorylocalDeviceId. This also still leaves the existing-global-versus-legacy-local-brain upgrade case unreconciled: an old project whose cluster points at its legacy device ID can become a viewer when some other project created the shared file first.Use create-if-absent semantics for the shared file, then re-read on
EEXIST, and reconcile project DB rows when a legacy local-brain ID differs from the selected shared ID.Sketch of the safer file-create pattern
- const created = randomUUID(); - writeTextAtomic(deviceIdPath, `${created}\n`); - return created; + const created = randomUUID(); + try { + fs.writeFileSync(deviceIdPath, `${created}\n`, { encoding: "utf8", flag: "wx" }); + return created; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") { + const racedExisting = fs.readFileSync(deviceIdPath, "utf8").trim(); + if (racedExisting.length > 0) return racedExisting; + } + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/deviceRegistryService.ts` around lines 146 - 158, readOrCreateLocalDeviceId currently races when multiple registries create deviceIdPath: change it to use create-if-absent semantics (attempt atomic create/open with exclusive flag for deviceIdPath), on EEXIST re-read the file to get the winner value instead of returning the locally generated UUID, and if a legacyProjectDeviceIdPath exists reconcile project DB rows when legacy !== selected shared ID (update any rows pointing at the legacy ID to the chosen shared ID) so in-memory localDeviceId and disk are consistent; reference readOrCreateLocalDeviceId, deviceIdPath, legacyProjectDeviceIdPath, randomUUID, writeTextAtomic and ensure error handling specifically handles EEXIST by re-reading and returning the actual disk value.
🤖 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.test.ts`:
- Around line 4540-4560: The test deletes a session that has no history, so it
won't catch failures to clear hydrated cache; before calling
service.deleteSession({ sessionId: session.id }) seed the session history (e.g.
call service.sendMessage or otherwise append an AgentChatEventEnvelope to the
session's transcript) so the session actually has events, then delete and assert
that service.getChatEventHistory(session.id).events is empty and truncated is
false; update the test around createSession, sendMessage (or direct transcript
seeding) and deleteSession accordingly to ensure deletion clears both in-memory
and hydrated-from-disk state.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 3692-3713: The readTranscriptEnvelopesForSessionId function uses
an unvalidated sessionId to build file paths
(transcriptsDir/${sessionId}.chat.jsonl and
chatTranscriptsDir/${sessionId}.jsonl) before confirming the id belongs to an
agent chat session; update the start of readTranscriptEnvelopesForSessionId to
trim and validate the sessionId by calling sessionService.get(trimmedId) and
checking isChatToolType(session?.toolType) (or return [] if invalid), then
proceed to use managedSessions, transcriptsDir and chatTranscriptsDir only after
this validation to prevent arbitrary path access.
- Around line 5662-5678: The early return when managed.runtime is falsy prevents
terminal invalidation from running and lets preserved Claude resume metadata
survive (e.g., idle_ttl then ended_session); change the logic so terminal
invalidation always runs even if managed.runtime is null: compute
preserveClaudeResumeState only when managed.runtime exists (use
managed.runtime?.kind or guard the kind check), but move or duplicate the call
that invalidates terminal reasons (the cleanup that handles
openCodeReason/ended_session) to execute before the early return (or remove the
early return and gate only the preserveClaudeResumeState-dependent reset).
Ensure references in the patch include managed.runtime,
preserveClaudeResumeState, and openCodeReason so reviewers can find the updated
flow.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 4412-4421: The IPC handler for IPC.agentChatGetEventHistory
forwards NaN because it only checks typeof maxEvents; update the handler
(ipcMain.handle callback that calls getCtx() and
ctx.agentChatService.getChatEventHistory) to guard maxEvents with
Number.isFinite and convert to an integer (e.g., Math.floor) or pass undefined
when invalid—i.e., replace the typeof arg?.maxEvents === "number" check with a
finite-number check and only pass a sanitized integer to getChatEventHistory so
NaN/Infinity are not forwarded.
In `@apps/desktop/src/main/services/sync/syncService.test.ts`:
- Line 40: The per-test host mock override used in the retry-test block is
missing the new setDiscoveryEnabled method, causing
hostService?.setDiscoveryEnabled(...) to throw; update the override in
syncService.test.ts (the mock returned in the retry-test override around the
507-546 block) to include setDiscoveryEnabled: vi.fn() (or the appropriate
jest/vi mock function) so it matches the default mock shape and prevents runtime
errors when tests toggle discovery.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Line 2385: The mock implementation for getEventHistory always returns
sessionId: "mock", which breaks session matching; update the getEventHistory
entry to echo the requested sessionId by using the same resolvedArg helper but
returning the sessionId from the incoming args (i.e., read args.sessionId or the
appropriate parameter passed to resolvedArg) instead of the hardcoded "mock" so
the browserMock mirrors the real API behavior.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1461-1463: Inside the if (!usedSnapshotPath) block in
AgentChatPane (where you call window.ade.sessions.get(sessionId)), clear the
loaded guard before any early return: when (!summary ||
!isChatToolType(summary.toolType)) set loadedHistoryRef.current = undefined (or
null) immediately prior to returning so a transient sessions.get() miss doesn't
permanently block retries; update the same block that references
usedSnapshotPath and loadedHistoryRef to ensure the guard is reset on this
fallback path.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 268-280: The label computation for missing lane groups uses
list[0]?.laneName without trimming, so blank or whitespace-only lane names
render as empty headers; update the logic where missingLaneSessionGroups is
mapped (the label variable used for the StickyGroupHeader) to trim
list[0]?.laneName and if the trimmed string is empty fall back to laneId,
ensuring StickyGroupHeader receives a non-blank label; keep the rest of the
props (sectionId, count, collapsed, onToggleCollapsed) unchanged.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 225-229: The host normalization currently strips everything after
the last ':' which mangles unbracketed IPv6 literals like "::1"; update the
logic in the host normalization block (the code that uses bracketEnd and the
subsequent colon check) to only treat the trailing ":port" form when there is
exactly one colon in the host string — e.g., count occurrences of ":" (or
compare firstIndex(of: ":") to lastIndex(of: ":")) and only run the host =
String(host[..<colon]) branch when there is a single colon; leave hosts with
multiple colons (unbracketed IPv6) untouched so the IPv6 allowlist can still
match.
In `@apps/ios/ADE/Views/Work/WorkSessionGrouping.swift`:
- Around line 152-158: The orphan lane label logic should trim whitespace and
fall back to the laneId when laneName is present but empty; update the
comparator that builds leftName/rightName (used in the sort) to trim characters
from left.value.first?.laneName and right.value.first?.laneName before using
them, and change the orphanEntries loop that computes label for WorkSessionGroup
(currently using list.first?.laneName ?? laneId) to use a trimmed value and fall
back to laneId when the trimmed string is empty (you can add a small helper like
orphanLaneLabel(sessions:fallback:) or inline trim + isEmpty check).
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 3384-3442: The test
testWorkSessionGroupsByLaneSurfacesOrphanLanesPerLaneId currently gives every
session the same startedAt via makeTerminalSessionSummary, so it doesn't
exercise orphan-lane ordering by latest startedAt; update the three orphan calls
(orphanOldSession, orphanNewSession, orphanNewSessionSibling) to pass distinct
startedAt values (e.g., orphanNew* with a later timestamp than orphanOld) when
calling makeTerminalSessionSummary so workSessionGroupsByLane's orphan sorting
by latest startedAt is actually tested while keeping the rest of the assertions
(ids, labels, and last?.sessions.count) unchanged.
---
Outside diff comments:
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Around line 914-930: publishTailnetDiscovery can race with in-flight unpublish
calls causing stale publishes to re-enable discovery; fix by serializing
unpublish/publish: introduce and track a shared Promise (e.g.,
activeUnpublishPromise) that unpublishTailnetDiscovery assigns to, have
publishTailnetDiscovery await that promise (or check its settled state) before
issuing a new publish, and when a publish completes check discoveryEnabled and,
if false, immediately trigger or await a fresh unpublishTailnetDiscovery to
force-off any stale external state; apply the same pattern to the other
publish/unpublish sites referenced (lines ~1025-1047 and ~2056-2078) to ensure
all serve off/publish operations are serialized.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1472-1491: The merge logic in AgentChatPane that builds merged
from parsed and the real-time tail (using eventsBySessionRef.current, existing,
parsed, tail, merged) must sort and deduplicate the tail before appending so
out-of-order or duplicate IPC events don't corrupt the event sequence used by
deriveRuntimeState(); update the tail construction to (1) sort by monotonic
sequence when present (falling back to timestamp) and (2) dedupe events (using a
stable unique key such as sequence if available, otherwise timestamp+sender/id)
so merged becomes [...parsed, ...sortedDedupedTail] (or parsed if tail empty).
Ensure you still prefer sequence ordering when available and keep
sessionId/AgentChatEventEnvelope semantics intact.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 4509-4562: The append-fast-path and the full dedup/sort path both
fail to respect the envelope.sequence for events that represent the same
instant; update the canAppendInOrder check and the comparator inside
deduplicatedChatEventHistory to treat equal parsed timestamps as ordered by
(sequence ?? 0) before falling back to string timestamps. Concretely: in the
canAppendInOrder closure (the code comparing last.timestamp and
envelope.timestamp using Self.parseIso8601) when envelopeDate == lastDate use
(envelope.sequence ?? 0) >= (last.sequence ?? 0) to decide appendability; and
inside deduplicatedChatEventHistory's sort comparator (where lhsDate == rhsDate)
first compare (lhs.sequence ?? 0) vs (rhs.sequence ?? 0) and only if equal fall
back to comparing timestamp strings.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 337-344: The test currently asserts maxBytes only before
disconnect; ensure the disconnect path preserves maxBytes by adding assertions
after calling service.disconnect(clearCredentials: false) that validate
service.chatSubscriptionPayloads().compactMap { $0["maxBytes"] as? Int } still
equals [2_000_000, 2_000_000] (and keep the existing checks on
subscribedChatSessionIds and sessionId payloads) so the subscription payloads
retain their maxBytes through the disconnect flow in ADETests.swift.
---
Duplicate comments:
In `@apps/desktop/src/main/services/sync/deviceRegistryService.ts`:
- Around line 146-158: readOrCreateLocalDeviceId currently races when multiple
registries create deviceIdPath: change it to use create-if-absent semantics
(attempt atomic create/open with exclusive flag for deviceIdPath), on EEXIST
re-read the file to get the winner value instead of returning the locally
generated UUID, and if a legacyProjectDeviceIdPath exists reconcile project DB
rows when legacy !== selected shared ID (update any rows pointing at the legacy
ID to the chosen shared ID) so in-memory localDeviceId and disk are consistent;
reference readOrCreateLocalDeviceId, deviceIdPath, legacyProjectDeviceIdPath,
randomUUID, writeTextAtomic and ensure error handling specifically handles
EEXIST by re-reading and returning the actual disk value.
🪄 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: 1e458e0a-ec94-4dbb-8f88-2523a3653ef2
⛔ Files ignored due to path filters (3)
docs/features/chat/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (24)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/sync/deviceRegistryService.test.tsapps/desktop/src/main/services/sync/deviceRegistryService.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/main/services/sync/syncService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/shared/ipc.tsapps/ios/ADE/Info.plistapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkSessionGrouping.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swift
Terminal teardown must still invalidate when runtime was already torn down - teardownRuntime no longer skips runtimeInvalidated/clearLaneDirectiveKey for terminal reasons (handle_close / ended_session / model_switch) when managed.runtime is already null. Previously: idle_ttl preserved, then ended_session returned early, leaving a stale sdkSessionId/lane key that a future resume could reattach to. - Added the mirror regression test to the preserve-across-shutdown one. - Fixes capy-ai + coderabbit Major on agentChatService.ts:5667/5678. getChatEventHistory validates sessionId before reading transcript paths - The new IPC-reachable history API builds filesystem paths from sessionId, so reject ids that aren't registered agent-chat sessions up front instead of trusting raw input. Fixes coderabbit Major on agentChatService.ts:3713. Device registry preserves per-project legacy IDs - If a project has a legacy per-project sync-device-id, always use it so existing iOS pairings and sync_cluster_state.brain_device_id stay valid; only write the shared file when it would agree. Prevents opening project B after A from silently adopting A's ID and dropping B into viewer mode. - Fixes capy-ai High on deviceRegistryService.ts:149. mergeEnvelopeStreams dedup by timestamp+type, not sequence - eventSequence restarts at 0 on session rebuild, so the same ordinals legitimately appear in the persisted transcript and the current-run buffer. Dedup on (timestamp, event.type) so fresh live envelopes aren't dropped as false duplicates on first hydration after process restart. - Fixes capy-ai Medium on agentChatService.ts:3738. iOS + renderer polish - Restore port 8788 to iOS tailnet candidates — desktop still falls back to 8788 when 8787 is busy (syncService.ts retry). - Preserve unbracketed IPv6 literals in syncNormalizedRouteHost (strip host:port only when there's exactly one colon). - Fall back from blank laneName to laneId in orphan-lane group labels on both desktop and iOS. - iOS orphan-lane ordering test uses distinct startedAt timestamps so chronological-sort is actually exercised. - AgentChatPane fallback early-return also clears loadedHistoryRef so a session-lookup miss no longer blocks future retries. - browserMock.getEventHistory echoes the requested sessionId. - syncService.test host-mock override stubs setDiscoveryEnabled. - agentChatService.test "drops history" now seeds history before delete so a regression that fails to clear the hydrated cache is actually caught. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Iteration 2 pushed as Critical fixes:
iOS / renderer polish:
All 443 chat + sync + terminals tests pass; typecheck clean. |
There was a problem hiding this comment.
Pull request overview
This PR improves chat/session continuity across desktop and iOS by preserving Claude resume metadata on non-terminal teardowns, surfacing sessions whose lane no longer exists (“orphan lanes”), and expanding/merging chat history snapshots to reduce event loss across reconnects and project switches.
Changes:
- Desktop: add a main-process
getChatEventHistorysnapshot API backed by an in-memory per-session ring buffer and use it in the renderer; preserve Claude V2 resume metadata across non-terminal teardowns; surface orphan-lane sessions inSessionListPane. - iOS: request larger chat snapshots (2 MB), merge truncated snapshots instead of replacing, retain up to 1,000 events, improve ordering/dedup, and normalize assistant text identity via
messageIdfallback. - Sync: centralize device identity to a shared path and gate host discovery (LAN + Tailnet) based on active project; update docs accordingly.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/features/terminals-and-sessions/ui-surfaces.md | Documents orphan-lane sticky groups in by-lane session list mode. |
| docs/features/sync-and-multi-device/ios-companion.md | Documents iOS 2 MB chat snapshot, merge-on-truncation, 1,000 event cap, and messageId fallback. |
| docs/features/chat/README.md | Documents Claude non-terminal teardown resume-state preservation behavior. |
| apps/ios/ADETests/ADETests.swift | Adds iOS tests for tailscale route detection, chat snapshot maxBytes, merge/replace semantics, timestamp ordering, messageId fallback, and orphan-lane grouping. |
| apps/ios/ADE/Views/Work/WorkTranscriptParser.swift | Falls back from itemId to messageId when parsing assistant text events. |
| apps/ios/ADE/Views/Work/WorkSessionGrouping.swift | Splits orphan sessions into per-lane groups and sorts by latest startedAt then name. |
| apps/ios/ADE/Views/Work/WorkEventMapping.swift | Uses trimmed itemId or messageId as a stable assistant text identity. |
| apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift | Prevents merging unidentified assistant text across tool/user events; documents invariant. |
| apps/ios/ADE/Views/Settings/SettingsPairingSection.swift | Updates pairing UI routing heuristics to use generalized tailscale-route detection. |
| apps/ios/ADE/Services/SyncService.swift | Adds tailscale route normalization, increases chat snapshot window, merge-on-truncation, dedup+sort+trim chat history. |
| apps/ios/ADE/Info.plist | Adds ATS exception domains for ade-sync and *.ts.net plaintext websocket connectivity. |
| apps/desktop/src/shared/ipc.ts | Adds IPC channel constant for agentChatGetEventHistory. |
| apps/desktop/src/renderer/components/terminals/SessionListPane.tsx | Renders orphan-lane sticky groups and sorts them by latest startedAt then name. |
| apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx | Adds a renderer test ensuring missing-lane sessions render in by-lane view. |
| apps/desktop/src/renderer/components/chat/AgentChatPane.tsx | Prefers snapshot IPC history loading with fallback to transcript tail; retries on failure. |
| apps/desktop/src/renderer/browserMock.ts | Mocks agentChat.getEventHistory in the browser mock. |
| apps/desktop/src/preload/preload.ts | Exposes agentChat.getEventHistory via preload bridge. |
| apps/desktop/src/preload/global.d.ts | Adds typings for window.ade.agentChat.getEventHistory. |
| apps/desktop/src/main/services/sync/syncService.ts | Threads localDeviceIdPath and hostDiscoveryEnabled through sync service; allows toggling discovery. |
| apps/desktop/src/main/services/sync/syncService.test.ts | Updates sync host mock to include setDiscoveryEnabled. |
| apps/desktop/src/main/services/sync/syncHostService.ts | Adds runtime-gated LAN/Tailnet discovery publishing and cleanup via setDiscoveryEnabled. |
| apps/desktop/src/main/services/sync/deviceRegistryService.ts | Supports shared device-id path with safe legacy-per-project migration/precedence. |
| apps/desktop/src/main/services/sync/deviceRegistryService.test.ts | Adds tests for shared device identity and legacy migration behavior. |
| apps/desktop/src/main/services/ipc/registerIpc.ts | Registers IPC handler for agentChatGetEventHistory with input validation. |
| apps/desktop/src/main/services/chat/agentChatService.ts | Adds in-memory event history ring buffer, transcript hydration, snapshot API, and Claude resume-state preservation rules. |
| apps/desktop/src/main/services/chat/agentChatService.test.ts | Adds tests for snapshot hydration/cleanup and Claude resume preservation across teardowns. |
| apps/desktop/src/main/main.ts | Uses shared userData device-id path and gates host discovery based on active project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…gments Claude V2 emits multiple text/reasoning deltas inside tight streaming loops, so two legitimate envelopes can legitimately share the same millisecond timestamp and event.type. The previous timestamp+type dedup key would collapse those distinct fragments into one on the first hydration, silently dropping the later fragment from the resumed history. Include a payload signature (JSON.stringify of the event) in the dedup key so true duplicates still collapse while distinct fragments at the same timestamp survive. Sequence stays out of the key since it restarts per run. Added regression test with two text envelopes at a shared timestamp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Iteration 3 pushed as
Not fixing in this PR (responding to the remaining two Medium flags):
|
Owner
Author
|
@codex review |
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
arul28
added a commit
that referenced
this pull request
Apr 23, 2026
Covers all work since v1.1.0 — folds in the iOS polish / App Store review prep originally planned for v1.1.1 plus the chat-continuity work from PR #175 (chat history on remount, Claude resume survival, orphan lanes, Tailscale routing cleanup). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6f246e103
ℹ️ 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".
| sessionId, | ||
| maxEvents: MAX_SELECTED_CHAT_SESSION_EVENTS, | ||
| }); | ||
| if (snapshot?.events?.length || snapshot?.sessionId === sessionId) { |
There was a problem hiding this comment.
This branch marks the snapshot path as successful whenever snapshot.sessionId === sessionId, even if the backend returned no events because the session lookup failed (e.g., transient project-switch timing). In that case loadedHistoryRef stays set and the pane stops retrying history hydration, recreating the “stuck empty chat until remount/new event” behavior. Please require a positive existence signal (or fall back when the snapshot is empty) before setting usedSnapshotPath.
Useful? React with 👍 / 👎.
Comment on lines
191
to
193
| static let hostCandidates = [ | ||
| "ade-sync", | ||
| "ade-desktop", | ||
| ] |
There was a problem hiding this comment.
Dropping "ade-desktop" from tailnet host candidates breaks backward compatibility for existing saved routes that still use that alias. After this change those addresses are no longer classified as tailnet, and the plaintext route filter rejects them, so previously paired devices can fail to reconnect unless users manually edit/re-pair. Keep the legacy alias in the candidate set while newer ade-sync routes roll out.
Useful? React with 👍 / 👎.
This was referenced Apr 24, 2026
Merged
Merged
Merged
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
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
sdkSessionId, lane directive,runtimeInvalidated) on non-terminal chat teardowns (idle_ttl,budget_eviction,pool_compaction,paused_run,project_close,shutdown) so the next turn re-attaches to the same V2 session instead of cold-starting.SessionListPaneand iOSworkSessionGroupsByLane, sorted by lateststartedAtwith name tiebreak and labeled with the session's preservedlaneName.chat_subscribe, merge truncated snapshots with existing events instead of replacing, retain up to 1,000 events per session, and fall back tomessageIdwhen assistant text is missingitemIdso Claude- and Codex-produced turns stay aligned.Test plan
node scripts/validate-docs.mjsxcodebuild build-for-testingon iPhone 17 Pro simulator; targeted tests green (orphan-lane grouping, chat merge, messageId fallback)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR preserves Claude V2 session resume metadata across non-terminal teardowns, surfaces orphaned-lane sessions as individual collapsible groups (desktop + iOS), grows the iOS chat history window to 2 MB with merge-on-truncation semantics, and centralizes device identity to a shared
userDatafile. The implementation is well-tested with targeted vitest and XCTest coverage across all new code paths.mergeEnvelopeStreams: the dedup key${timestamp}#${event.type}is too coarse. Rapid streaming text events within the same millisecond are treated as duplicates and the later fragments are silently dropped, which would manifest as truncated assistant replies after a project switch triggers a disk/memory merge.Confidence Score: 4/5
Safe to merge after addressing the mergeEnvelopeStreams dedup key, which can silently drop streaming text fragments during post-project-switch history hydration.
All other changes are well-structured and well-tested. The single P1 finding (timestamp+type dedup collision) is a correctness issue on a new code path — it does not break existing behaviour but can silently truncate chat history after an idle_ttl teardown and project switch, which is exactly the scenario this PR is meant to fix.
apps/desktop/src/main/services/chat/agentChatService.ts — specifically the mergeEnvelopeStreams dedup key around line 397
Important Files Changed
Sequence Diagram
sequenceDiagram participant R as Renderer participant IPC participant ACS as AgentChatService participant FS as Filesystem Note over ACS: idle_ttl fires ACS->>ACS: teardownRuntime(idle_ttl) ACS->>ACS: preserveClaudeResumeState = true ACS->>FS: persistChatState(sdkSessionId, laneDirectiveKey) ACS->>ACS: runtimeInvalidated = false Note over R: User switches back to project/tab R->>IPC: agentChat.getEventHistory(sessionId, maxEvents) IPC->>ACS: getChatEventHistory(sessionId) ACS->>ACS: Validate sessionId via sessionService alt Not yet hydrated ACS->>FS: readTranscriptEnvelopesForSessionId ACS->>ACS: mergeEnvelopeStreams(disk, memory buffer) ACS->>ACS: Mark hydrated end ACS-->>IPC: events + truncated flag IPC-->>R: Render full history Note over R: User sends next message R->>IPC: agentChat.runSessionTurn IPC->>ACS: runSessionTurn ACS->>ACS: Read persisted sdkSessionId ACS->>ACS: unstable_v2_resumeSession(sdkSessionId) Note over ACS: Session re-attaches to same V2 contextComments Outside Diff (3)
apps/desktop/src/renderer/components/terminals/SessionListPane.tsx, line 210-211 (link)Math.max(...sessions.map(...))with invalid dates producesNaNlatestStartedAtspreads the mapped timestamps intoMath.max. If anysession.startedAtis malformed (e.g.,""),new Date(...).getTime()returnsNaN, andMath.maxpropagatesNaNto the comparator. The sort callback then receivesNaN - NaN = NaN, which causes unpredictable ordering in V8'sArray.sort. Thesessions.length > 0guard prevents the zero-element case (-Infinity), but not the bad-date case. A small defensive fix:Prompt To Fix With AI
apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift, line 352-357 (link)previousEnvelopeWasAssistantTextflag invariant is implicitThe logic is correct for current variants, but if a new
WorkChatEventcase is added without resetting the flag, the merge guard could fire incorrectly. Consider adding a comment at the reset sites to make the invariant explicit for future contributors.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
apps/desktop/src/main/services/chat/agentChatService.ts, line 397-400 (link)timestamp#typededup key can silently drop streaming text fragmentsmergeEnvelopeStreamskeys each envelope as${entry.timestamp}#${entry.event.type}. Claude streaming turns routinely emit manytextevents in quick succession; if two fragments are timestamped within the same millisecond the second is permanently dropped from the merged history. This would silently produce a truncated assistant reply every time a user returns to a project after anidle_ttlteardown.A more collision-resistant key that stays cross-run stable would incorporate
sequenceas a secondary discriminator (sequence restarts per-run, but within a single run every event that lands in the in-memory buffer has a unique sequence; for the disk-only path, sequence is also monotone). Alternatively, keying on the first ~64 bytes of text content fortextevents would be sufficient to distinguish adjacent fragments.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "Address second round of review feedback" | Re-trigger Greptile