Make remote sessions recover reliably under load#869
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR updates relay endpoint retention, desktop runtime services, sync protocols, project icon and usage workers, GitHub and Codex resume handling, webclient project boundaries, packaging validation, and iOS synchronization and timeline behavior. ChangesRelay Retention
Desktop Runtime and Project Services
Sync and Webclient
iOS
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/github/githubService.ts (2)
121-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
readGhHostsFileToken's process-wide cache is never invalidated onforceRefresh.
cacheGhHostsToken/processGhHostsTokenCachecorrectly bounds and TTLs the hosts.yml lookup, but nothing incomputeStatus'sforceRefreshbranch (lines 1040-1047),setToken, orclearTokenclears this module-level cache. SincereadGitHubCliAuthTokenchecksreadGhHostsFileToken()before ever exec'inggh auth tokenand returns immediately if a (possibly stale) cached token is found, a user who rotates their token viagh auth login/gh auth refreshoutside ADE won't see it reflected by "Force Refresh" in Settings until the hosts-cache TTL naturally expires — the explicit forceRefresh contract is silently defeated for this fallback path.🐛 Proposed fix: clear the hosts cache on forceRefresh
function cacheGhHostsToken(hostsPath: string, token: string | null): void { ... } + +function clearGhHostsTokenCache(): void { + processGhHostsTokenCache.clear(); +}const computeStatus = async (opts: { forceRefresh?: boolean } = {}): Promise<GitHubStatus> => { if (opts.forceRefresh) { cachedStatus = null; cachedAt = 0; cachedStatusTokenDigest = null; sharedGhAuth.authCache = null; sharedGhAuth.statusCache.clear(); + clearGhHostsTokenCache(); }🤖 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/github/githubService.ts` around lines 121 - 164, Invalidate processGhHostsTokenCache at the start of computeStatus’s forceRefresh branch so readGhHostsFileToken cannot return a stale hosts.yml token during an explicit refresh. Reuse the existing cache symbol and preserve normal TTL-based caching for non-refresh calls; do not change setToken or clearToken behavior unless needed to support this forceRefresh invalidation.
654-685: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
readAuthTokenSyncbypassesADE_DISABLE_GH_AUTH_FALLBACK, unlike the asyncreadAuthToken/readGhAuthToken/readGitHubCliAuthTokenpaths.Both
readGhAuthToken(line 607-610) andreadGitHubCliAuthToken(line 154-156) explicitly short-circuit to "no token" whenADE_DISABLE_GH_AUTH_FALLBACK === "1".readAuthTokenSync, however, unconditionally callsreadGhHostsFileToken()whenever there's no validsharedGhAuth.authCacheentry, with no check of that env var. This means the synchronousgetTokenOrThrow()can silently resolve a token sourced fromhosts.ymleven when the flag is set specifically to disable that fallback forgetTokenOrThrowAsync()/readAuthToken()— a real behavioral inconsistency between the sync and async token getters that share the same conceptual contract.🐛 Proposed fix: honor the disable flag in the sync path too
const readAuthTokenSync = (): GitHubTokenLookup => { const primary = readPrimaryAuthToken(); if (primary) return primary; + if (process.env.ADE_DISABLE_GH_AUTH_FALLBACK === "1") { + return { token: null, ghCliPath: null, ghAuthError: null, source: "none", patTokenStored: false }; + } const cachedGh = sharedGhAuth.authCache && sharedGhAuth.authCache.expiresAt > Date.now() ? sharedGhAuth.authCache : null;🤖 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/github/githubService.ts` around lines 654 - 685, Update readAuthTokenSync so it honors ADE_DISABLE_GH_AUTH_FALLBACK before calling readGhHostsFileToken, matching the existing behavior in readAuthToken and readGhAuthToken. When the flag is set to "1", skip hosts.yml fallback and preserve the no-token result unless a valid cached authentication entry is available.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx (1)
575-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the continuation launch→PTY payload builder. Both continuation entry points hand-build the same conditional
sendArgsfields (model, reasoningEffort,fastModewithcodexFastModefallback, permissionMode, codexApprovalPolicy/Sandbox/ConfigSource). Keeping two copies risks the codex-config/fast-mode forwarding silently diverging between the Work-tab and Lane paths. Extract a shared helper (e.g.buildContinuationLaunchArgs(launch)) returning the partial payload and spread it at both sites.
apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx#L575-L590: replace the inline...(launch?...)spreads with...buildContinuationLaunchArgs(launch).apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts#L789-L804: replace the identical inline spreads with the same helper.🤖 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/terminals/TerminalsPage.tsx` around lines 575 - 590, The continuation launch payload fields are duplicated across both PTY entry points. Add a shared buildContinuationLaunchArgs(launch) helper preserving the existing model, reasoningEffort, fastMode with codexFastMode fallback, permission, sandbox, and config-source mappings, then spread its result at apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx lines 575-590 and apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts lines 789-804, replacing the duplicated inline spreads at both sites.apps/desktop/src/main/services/lanes/laneListSnapshotService.ts (1)
74-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared lane cache-key serialization. Both key builders serialize the identical lane field set (
id,parentLaneId,branchRef,baseRef,worktreePath,archivedAt) sorted byid. Keeping two copies risks the caches keying on different fields over time, silently diverging cache-hit behavior. Consider a shared helper (e.g.serializeLaneCacheFields(lanes)) reused by both.
apps/desktop/src/main/services/lanes/laneListSnapshotService.ts#L74-L93: call the shared helper for thelanesportion, keeping the option-flag fields local tooptionalLaneEnrichmentKey.apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts#L31-L43: call the shared helper, retaining the"default"branch for the no-lanes case.🤖 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/lanes/laneListSnapshotService.ts` around lines 74 - 93, Extract the shared lane-field serialization into a helper such as serializeLaneCacheFields and reuse it in both key builders; in apps/desktop/src/main/services/lanes/laneListSnapshotService.ts lines 74-93, use it for the lanes portion while keeping option flags local to optionalLaneEnrichmentKey, and in apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts lines 31-43, use it while preserving the "default" no-lanes branch.apps/ios/ADETests/ADETests.swift (1)
15948-15986: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name contradicts its own assertion.
testWorkFilteredSessionsHidesStaleStandaloneCliRowsButKeepsChatOwnedShellsnow assertsfiltered.map(\.id) == ["chat-parent", "shell-child", "legacy-cli"]— i.e. the stale standalone CLI row (legacy-cli) is kept, not hidden. The function name still promises it's hidden, which will mislead anyone debugging a future regression in this filter.✏️ Suggested rename
- func testWorkFilteredSessionsHidesStaleStandaloneCliRowsButKeepsChatOwnedShells() { + func testWorkFilteredSessionsShowsStaleStandaloneCliRowsAndKeepsChatOwnedShells() {🤖 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/ADETests/ADETests.swift` around lines 15948 - 15986, Rename testWorkFilteredSessionsHidesStaleStandaloneCliRowsButKeepsChatOwnedShells to accurately reflect that the filtered results retain the stale standalone CLI row alongside the chat-owned shell. Keep the test setup and assertion unchanged.apps/desktop/src/main/services/github/githubService.ts (1)
785-857: 🚀 Performance & Scalability | 🔵 TrivialLGTM! The forceRefresh de-dup path (await the in-flight probe, re-check for a newer one, then delete-and-recompute) correctly handles the common races; the transient-failure cooldown and bounded
statusCacheeviction are also sound, and match the added test coverage (retries transient status failures after a short shared cooldown,does not extend the shared cooldown for an invalid gh token).One minor edge case worth noting for awareness: if two
forceRefreshcalls race with no existing in-flight entry for the key, both will proceed to compute independently (the secondstatusInFlight.setoverwrites the first), so a duplicate network probe can occur. This is a minor inefficiency, not a correctness bug, so no action needed unless duplicate GitHub API calls under refresh races become a real cost concern.🤖 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/github/githubService.ts` around lines 785 - 857, Consider the forceRefresh branch in readSharedGithubStatusProbe: concurrent refreshes with no existing in-flight entry can start duplicate probes because each overwrites statusInFlight. If deduplication is required, coordinate refresh callers through a shared in-flight promise before starting computeGithubStatusProbe, while preserving the existing newer-refresh and cache invalidation behavior.apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
21922-21927: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
eventmember in the inline type-predicate literal.The type-predicate return annotation repeats the same
event: Extract<...>member twice:(entry): entry is AgentChatEventEnvelope & { event: Extract<AgentChatEventEnvelope["event"], { type: "approval_request" }>; event: Extract<AgentChatEventEnvelope["event"], { type: "approval_request" }>; } => entry.event.type === "approval_request",Looks like an accidental copy/paste duplicate line rather than intentional. Even if this doesn't produce a hard compiler error (duplicate-key checks are strictest for object literals/interfaces, less so for anonymous type literals), it's dead, confusing code that should be removed.
🧹 Proposed fix
const liveRingEvent = await waitForEvent( emitted, (entry): entry is AgentChatEventEnvelope & { - event: Extract<AgentChatEventEnvelope["event"], { type: "approval_request" }>; event: Extract<AgentChatEventEnvelope["event"], { type: "approval_request" }>; } => entry.event.type === "approval_request", );🤖 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.test.ts` around lines 21922 - 21927, Remove the duplicated event member from the inline type predicate used by waitForEvent in the liveRingEvent declaration, leaving a single event: Extract<...> property while preserving the approval_request type guard.
🤖 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/localRuntime/localRuntimeTimeoutPolicy.ts`:
- Line 2: Increase LOCAL_RUNTIME_IPC_PROJECT_SETUP_TIMEOUT_MS to cover two
120-second ensureProject attempts plus reasonable headroom, preventing the setup
budget from expiring during a valid retry. Update the related timeout tests to
assert the new budget and preserve existing timeout behavior elsewhere.
In `@apps/desktop/src/main/services/usage/usageTrackingService.ts`:
- Around line 2942-2951: The provider refresh gate in the usage tracking flow
must also trigger when project-scoped costs are not ready. Update
providerNeedsRefresh to account for projectCostsReady when scope is "project",
while preserving the existing missing-history behavior and avoiding refreshes
for ready project costs.
---
Outside diff comments:
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 121-164: Invalidate processGhHostsTokenCache at the start of
computeStatus’s forceRefresh branch so readGhHostsFileToken cannot return a
stale hosts.yml token during an explicit refresh. Reuse the existing cache
symbol and preserve normal TTL-based caching for non-refresh calls; do not
change setToken or clearToken behavior unless needed to support this
forceRefresh invalidation.
- Around line 654-685: Update readAuthTokenSync so it honors
ADE_DISABLE_GH_AUTH_FALLBACK before calling readGhHostsFileToken, matching the
existing behavior in readAuthToken and readGhAuthToken. When the flag is set to
"1", skip hosts.yml fallback and preserve the no-token result unless a valid
cached authentication entry is available.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 21922-21927: Remove the duplicated event member from the inline
type predicate used by waitForEvent in the liveRingEvent declaration, leaving a
single event: Extract<...> property while preserving the approval_request type
guard.
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 785-857: Consider the forceRefresh branch in
readSharedGithubStatusProbe: concurrent refreshes with no existing in-flight
entry can start duplicate probes because each overwrites statusInFlight. If
deduplication is required, coordinate refresh callers through a shared in-flight
promise before starting computeGithubStatusProbe, while preserving the existing
newer-refresh and cache invalidation behavior.
In `@apps/desktop/src/main/services/lanes/laneListSnapshotService.ts`:
- Around line 74-93: Extract the shared lane-field serialization into a helper
such as serializeLaneCacheFields and reuse it in both key builders; in
apps/desktop/src/main/services/lanes/laneListSnapshotService.ts lines 74-93, use
it for the lanes portion while keeping option flags local to
optionalLaneEnrichmentKey, and in
apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts lines 31-43, use
it while preserving the "default" no-lanes branch.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 575-590: The continuation launch payload fields are duplicated
across both PTY entry points. Add a shared buildContinuationLaunchArgs(launch)
helper preserving the existing model, reasoningEffort, fastMode with
codexFastMode fallback, permission, sandbox, and config-source mappings, then
spread its result at
apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx lines 575-590
and apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts lines
789-804, replacing the duplicated inline spreads at both sites.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 15948-15986: Rename
testWorkFilteredSessionsHidesStaleStandaloneCliRowsButKeepsChatOwnedShells to
accurately reflect that the filtered results retain the stale standalone CLI row
alongside the chat-owned shell. Keep the test setup and assertion unchanged.
🪄 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: 29ebe679-0b27-422c-957e-92768e0bc00f
⛔ Files ignored due to path filters (13)
CHANGELOG.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.jsonchangelog/index.mdxis excluded by!changelog/**changelog/v1.2.34.mdxis excluded by!changelog/**docs.jsonis excluded by!docs.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/project-home/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (98)
apps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/jsonrpc.test.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/projects/projectCatalog.tsapps/ade-cli/src/services/projects/projectIconResolver.test.tsapps/ade-cli/src/services/projects/projectIconResolver.tsapps/ade-cli/src/services/projects/projectScope.test.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/tsup.config.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/main/services/git/git.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/ipcTimeouts.test.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.tsapps/desktop/src/main/services/projects/projectIconResolver.test.tsapps/desktop/src/main/services/projects/projectIconResolver.tsapps/desktop/src/main/services/projects/projectIconThumbnail.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/main/services/usage/usageLedgerWorker.tsapps/desktop/src/main/services/usage/usageLedgerWorkerClient.test.tsapps/desktop/src/main/services/usage/usageLedgerWorkerClient.tsapps/desktop/src/main/services/usage/usageStatsStore.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/renderer/components/account/AccountPage.test.tsxapps/desktop/src/renderer/components/account/AccountPage.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/agentChat.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/renderer/webclient/adapter/infra/chatEventDedup.tsapps/desktop/src/renderer/webclient/adapter/infra/invalidation.tsapps/desktop/src/renderer/webclient/adapter/infra/registries.tsapps/desktop/src/renderer/webclient/adapter/personalChats.tsapps/desktop/src/renderer/webclient/adapter/sessionsPty.tsapps/desktop/src/renderer/webclient/adapter/types.tsapps/desktop/src/renderer/webclient/shell/WebClientRoot.tsxapps/desktop/src/renderer/webclient/shell/__tests__/WebClientRoot.test.tsxapps/desktop/src/renderer/webclient/sync/__tests__/sync.test.tsapps/desktop/src/renderer/webclient/sync/client.tsapps/desktop/src/renderer/webclient/sync/connection.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/types/externalSessions.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/desktop/tsup.config.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/SyncRecoveryPolicyTests.swift
|
@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: 046cbce152
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (1)
1275-1281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
localRuntimeActionTimeoutMsinstead of duplicating the daemon-timeout logic.This inline selection is an exact copy of
localRuntimeActionTimeoutMs(domain, action)inlocalRuntimeTimeoutPolicy.ts. The IPC layer already derives its budget from that helper (localRuntimeActionIpcTimeoutMs), so keeping a second hand-rolled copy here risks the daemon-side and renderer-side budgets silently diverging if the fallback rules change.♻️ Proposed refactor
- const actionKey = `${request.domain}.${request.action}`; - const actionCallOptions = { - timeoutMs: longRunningLocalRuntimeActionTimeoutMs(actionKey) - ?? (request.domain === "file" - ? LOCAL_RUNTIME_FILE_ACTION_TIMEOUT_MS - : LOCAL_RUNTIME_ACTION_TIMEOUT_MS), - }; + const actionCallOptions = { + timeoutMs: localRuntimeActionTimeoutMs(request.domain, request.action), + };Update the import block (lines 43-48) to pull in
localRuntimeActionTimeoutMs;longRunningLocalRuntimeActionTimeoutMs,LOCAL_RUNTIME_FILE_ACTION_TIMEOUT_MS, andLOCAL_RUNTIME_ACTION_TIMEOUT_MScan be dropped if unused elsewhere in this file.🤖 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/localRuntime/localRuntimeConnectionPool.ts` around lines 1275 - 1281, Update the actionCallOptions construction near actionKey to use localRuntimeActionTimeoutMs(request.domain, request.action) instead of duplicating the inline fallback selection. Adjust the imports by adding localRuntimeActionTimeoutMs and removing longRunningLocalRuntimeActionTimeoutMs, LOCAL_RUNTIME_FILE_ACTION_TIMEOUT_MS, and LOCAL_RUNTIME_ACTION_TIMEOUT_MS if no longer used elsewhere in the file.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1275-1281: Update the actionCallOptions construction near
actionKey to use localRuntimeActionTimeoutMs(request.domain, request.action)
instead of duplicating the inline fallback selection. Adjust the imports by
adding localRuntimeActionTimeoutMs and removing
longRunningLocalRuntimeActionTimeoutMs, LOCAL_RUNTIME_FILE_ACTION_TIMEOUT_MS,
and LOCAL_RUNTIME_ACTION_TIMEOUT_MS if no longer used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27ecd56c-69dc-4e33-9e98-e00afd628983
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!*.mdchangelog/v1.2.34.mdxis excluded by!changelog/**
📒 Files selected for processing (19)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/ipcTimeouts.test.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/lanes/laneCacheKey.test.tsapps/desktop/src/main/services/lanes/laneCacheKey.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/localRuntime/localRuntimeTimeoutPolicy.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/desktop/src/main/services/lanes/laneListSnapshotService.ts
- apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/main/services/usage/usageTrackingService.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/terminals/cliLaunch.ts (1)
78-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
recoverImportedContinuationLaunch's caching/TTL/dedup behavior.Only
mergeContinuationLaunchappears covered by tests per the provided context; the cache dedup, TTL-expiry re-fetch, and size-bound eviction logic inrecoverImportedContinuationLaunchhas no visible tests, despite being the more subtle/stateful piece (see the race condition flagged above).🤖 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/terminals/cliLaunch.ts` around lines 78 - 115, Add tests for recoverImportedContinuationLaunch covering concurrent calls deduplicating to one list request, cached results being reused within CONTINUATION_LAUNCH_LOOKUP_TTL_MS, expired entries triggering a fresh request, and eviction when continuationLaunchLookups exceeds CONTINUATION_LAUNCH_LOOKUP_MAX_ENTRIES. Also verify failed requests remove their cache entry so later calls can retry.
🤖 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/scripts/ensure-ade-cli-build.cjs`:
- Around line 58-63: Update oldestMtimeMs around fs.readdirSync so
directory-read errors are caught and return 0, matching the existing failed
statSync fallback; preserve the current empty-directory and recursive reduction
behavior when reading succeeds.
In `@apps/desktop/scripts/packaged-ade-cli-resources.cjs`:
- Around line 20-29: Expand directory sources into concrete destination-relative
payload files before validation. In
apps/desktop/scripts/packaged-ade-cli-resources.cjs:20-29, add and use a
recursive expansion helper; in
apps/desktop/scripts/after-pack-runtime-fixes.cjs:394-399 and
apps/desktop/scripts/validate-mac-artifacts.mjs:37-41, require every expanded
file; in apps/desktop/scripts/validate-win-artifacts.mjs:38-42, derive expected
artifacts from those files, and at 205-207 retain a concrete required-payload
check instead of accepting any non-empty ADE CLI resource configuration.
In `@apps/desktop/src/renderer/components/terminals/cliLaunch.ts`:
- Around line 95-104: Update the catch handler in the continuation launch lookup
flow to delete the cache entry only when it still references the rejecting
request’s own promise. Preserve newer entries created after TTL expiration,
while retaining the existing error propagation and cleanup behavior for the
current entry.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/terminals/cliLaunch.ts`:
- Around line 78-115: Add tests for recoverImportedContinuationLaunch covering
concurrent calls deduplicating to one list request, cached results being reused
within CONTINUATION_LAUNCH_LOOKUP_TTL_MS, expired entries triggering a fresh
request, and eviction when continuationLaunchLookups exceeds
CONTINUATION_LAUNCH_LOOKUP_MAX_ENTRIES. Also verify failed requests remove their
cache entry so later calls can retry.
🪄 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: 20d41dc1-8f3f-414b-88b4-99a13f3f2e4f
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/usage-tracking.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (25)
apps/ade-cli/scripts/verify-built-cli.mjsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/tsup.config.tsapps/desktop/package.jsonapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/ensure-ade-cli-build.cjsapps/desktop/scripts/packaged-ade-cli-resources.cjsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/usage/usageLedgerWorker.tsapps/desktop/src/main/services/usage/usageLedgerWorkerClient.test.tsapps/desktop/src/main/services/usage/usageLedgerWorkerClient.tsapps/desktop/src/main/services/usage/usageLedgerWorkerEntry.tsapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/tsup.config.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/tsup.config.ts
- apps/ade-cli/tsup.config.ts
- apps/ade-cli/src/cli.ts
- apps/desktop/src/main/services/usage/usageLedgerWorker.ts
- apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts
- apps/desktop/src/main/services/usage/usageLedgerWorkerClient.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05af0a1158
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b39d902606
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5400562406
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a43c4eab0b
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Validation
Release
Prepared for desktop v1.2.34 and iOS 1.1.10 build 37.
Summary by CodeRabbit
Greptile Summary
This PR improves remote-session recovery and responsiveness under load. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
The restarted chat-stream fixes preserve distinct events when sequence numbers restart. The transcript recovery path keeps restored old bytes on their previous logical offset. No blocking issue was found in the updated fixes.
No files need additional attention.
What T-Rex did
Important Files Changed
Reviews (8): Last reviewed commit: "fix: normalize persisted Codex permissio..." | Re-trigger Greptile