Improve iOS chat responsiveness - #919
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 52 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 Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesiOS chat flow
CLI launcher test adjustment
Estimated code review effort: 4 (Complex) | ~45 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 |
arul28
changed the title
f10ae37 to
9da4aee
Compare
|
@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: 9da4aee53e
ℹ️ 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".
![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 (1)
apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift (1)
693-716: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore
initialTranscriptTailHydratedbefore re-caching transcript/fallback.
setTranscript/setFallbackEntries(called at Lines 710/713) internally callcacheCurrentTranscriptPresentationIfNeeded(), which persistsinitialTranscriptTailHydratedfrom the current@Statevalue. SinceinitialTranscriptTailHydrated = cached.initialTranscriptTailHydratedruns at Line 715, after those calls, the presentation cache entry gets briefly re-written with the stale (pre-restore) hydration flag. If the view disappears in that narrow window before the next natural cache write (later inloadTranscript), the persisted cache permanently loses the correct hydration state — causing an unnecessary opening-snapshot re-request (and the 2 MB download) on the next reopen, which is exactly what this PR is trying to avoid.Move the assignment above the
setTranscript/setFallbackEntriescalls so the intermediate cache write already carries the correct value.🐛 Proposed fix
olderTranscriptCursor = cached.olderTranscriptCursor transcriptCursorKind = cached.transcriptCursorKind olderChatEventHistoryCursor = cached.olderChatEventHistoryCursor transcriptEntriesByIndex = workChatTranscriptEntriesByIndexForRestoredPresentation( fallbackEntries: cached.fallbackEntries, cursorKind: cached.transcriptCursorKind ) + initialTranscriptTailHydrated = cached.initialTranscriptTailHydrated if !cached.transcript.isEmpty { setTranscript(cached.transcript) } if !cached.fallbackEntries.isEmpty { setFallbackEntries(cached.fallbackEntries) } - initialTranscriptTailHydrated = cached.initialTranscriptTailHydrated }🤖 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/WorkSessionDestinationView.swift` around lines 693 - 716, In seedTranscriptFromPresentationCacheIfNeeded(), assign initialTranscriptTailHydrated from cached.initialTranscriptTailHydrated before calling setTranscript or setFallbackEntries, so their internal cache writes persist the restored hydration state; remove the later assignment.
🧹 Nitpick comments (2)
apps/ios/ADE/Views/Work/WorkChatSessionView.swift (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the SwiftUI state private.
Change this to
@State private var contextUsageViewModelCache. SwiftLint already reportsprivate_swiftui_state; view-owned state should not be exposed through the view’s internal API. As per path instructions, this iOS SwiftUI state should follow SwiftUI ownership conventions.🤖 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/WorkChatSessionView.swift` at line 158, Update the contextUsageViewModelCache declaration in WorkChatSessionView to make the `@State` property private, preserving its existing initialization and behavior.Sources: Path instructions, Linters/SAST tools
apps/ios/ADE/Services/SyncService.swift (1)
8394-8447: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the pending-unsubscribe capacity bound into a named constant.
scheduleChatEventUnsubscribehardcodes the retention bound (4) inline, whereas the siblingchatSnapshotRequestCoalescingIntervalis a named constant. Extracting achatEventUnsubscribeRetentionLimitconstant would make the bound easier to find/tune and keep the two related knobs consistent.♻️ Proposed refactor
private let chatSnapshotRequestCoalescingInterval: TimeInterval = 5 + private let chatEventUnsubscribeRetentionLimit = 4 private var recentFullChatSnapshotRequestAtBySession: [String: Date] = [:]- while pendingChatUnsubscribesBySession.count >= 4, + while pendingChatUnsubscribesBySession.count >= chatEventUnsubscribeRetentionLimit,🤖 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/Services/SyncService.swift` around lines 8394 - 8447, Define a named chatEventUnsubscribeRetentionLimit constant alongside chatSnapshotRequestCoalescingInterval, then update scheduleChatEventUnsubscribe to use it instead of the inline 4 capacity bound. Preserve the existing retention behavior and eviction logic.
🤖 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/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.ts`:
- Line 550: The test around openRemoteRpcSession currently only verifies that
spawnProcess was called, allowing excessive retry attempts to pass. Replace this
assertion with an upper-bound assertion on spawnProcess call count tied to the
expected route or retry attempt count, preferably deriving it from the route
matrix rather than using an arbitrary hard-coded limit.
In `@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift`:
- Around line 3566-3603: Scope WorkContextUsageViewModelCache to the chat
session by including sessionId in Key and passing it through value, ensuring
cached entries cannot be reused across sessions with matching
transcriptRenderSignature. Update callers such as contextUsageViewModelCache
usage accordingly and add a regression test that switches sessions while reusing
the render signature and verifies fresh context usage is returned.
---
Outside diff comments:
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift`:
- Around line 693-716: In seedTranscriptFromPresentationCacheIfNeeded(), assign
initialTranscriptTailHydrated from cached.initialTranscriptTailHydrated before
calling setTranscript or setFallbackEntries, so their internal cache writes
persist the restored hydration state; remove the later assignment.
---
Nitpick comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 8394-8447: Define a named chatEventUnsubscribeRetentionLimit
constant alongside chatSnapshotRequestCoalescingInterval, then update
scheduleChatEventUnsubscribe to use it instead of the inline 4 capacity bound.
Preserve the existing retention behavior and eviction logic.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift`:
- Line 158: Update the contextUsageViewModelCache declaration in
WorkChatSessionView to make the `@State` property private, preserving its existing
initialization and behavior.
🪄 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 Plus
Run ID: b81bce64-5e17-4a5f-91a6-b24084538316
⛔ Files ignored due to path filters (1)
docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (8)
apps/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 550d60a143
ℹ️ 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: 2dcc44b9ba
ℹ️ 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. 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". |
1 participant
Summary
Verification
Summary by CodeRabbit