Refine pending input cards across ADE surfaces#603
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
@copilot review but do not make fixes |
|
Warning Review limit reached
More reviews will be available in 17 minutes and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds provider-branded pending input UX across Desktop (React), iOS (SwiftUI), and the ADE CLI. Introduces shared ChangesProvider-branded pending input UX
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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: 0b2cb51cb6
ℹ️ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swift (1)
111-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing scroll anchor id on pending plan approval card breaks blocking-gate auto-scroll
WorkChatSessionViewcomputes a plan-approval anchor as"pending-question-\(plan.id)", but this view branch never sets that.id(...), soproxy.scrollTo(...)cannot target it. Add the same id pattern here to keep haptic+scroll behavior consistent for plan approvals.Suggested patch
case .pendingPlanApproval(let plan): WorkPlanReviewCard( plan: plan, busy: actionInFlight || !isLive, onDecision: { decision, feedback in await runSessionAction { // Approve: send "accept" decision directly. // Reject: send "decline"; if the user typed feedback, also // queue it as a follow-up steer message so the agent sees the // revision notes in the next turn. await onApproveRequest(plan.id, decision) if decision == .decline, let feedback, !feedback.isEmpty { _ = await onSend(feedback) } } }, fallbackProvider: chatSummary?.provider ) + .id("pending-question-\(plan.id)")🤖 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`+Timeline.swift around lines 111 - 128, The WorkPlanReviewCard in the .pendingPlanApproval case is missing the scroll anchor id modifier, which breaks the auto-scroll functionality for blocking-gate approval cards. Add a .id() modifier to the WorkPlanReviewCard using the pattern "pending-question-\(plan.id)" to match the anchor that WorkChatSessionView computes and uses with proxy.scrollTo(), ensuring the haptic and scroll behavior remains consistent across all plan approval interactions.apps/desktop/src/main/services/adeActions/registry.test.ts (1)
955-962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnapshot fixture asserts an exact pending count without a pending item id.
The mocked awaiting chat lacks
pendingInputItemId, but Line 1010 expectspendingInputCount: 1. For the “exact pending input requests” contract, add a concretependingInputItemIdin the fixture (or expect0).Suggested fixture adjustment
agentChatService: { listSessions: vi.fn(async () => [ { sessionId: "awaiting-chat", status: "active", awaitingInput: true, + pendingInputItemId: "pending-1", identityKey: null, }, ]), },Also applies to: 1006-1011
🤖 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/adeActions/registry.test.ts` around lines 955 - 962, The mocked listSessions function in the test fixture is missing the pendingInputItemId property in the session object, but the assertion expects pendingInputCount to be 1. To fix this, either add a concrete pendingInputItemId value to the mock session object within the listSessions mock function definition, or update the corresponding assertion to expect pendingInputCount: 0 to align with a fixture that lacks a pending input item identifier.
🧹 Nitpick comments (2)
apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx (1)
34-47: ⚡ Quick winCentralize provider-accent eligibility to prevent cross-surface drift.
PROVIDER_ACCENT_SOURCESduplicates provider knowledge that is already modeled elsewhere (shared label/type paths). This can drift and produce mixed UX (provider-specific header label but fallback violet accent) when a provider is added in one place but not this set. Prefer a single shared resolver for source normalization + accent mapping used by Desktop/CLI/iOS-facing layers.🤖 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/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx` around lines 34 - 47, The PROVIDER_ACCENT_SOURCES set in the pendingInputAccent function duplicates provider knowledge that already exists in the AdeCodeProvider type definition, creating a maintenance risk where providers could be added to the type but not to this set. Replace the hardcoded PROVIDER_ACCENT_SOURCES set with a call to a shared provider validation function (or create one if it doesn't exist) that checks whether a given provider should receive custom accent styling. This shared function should be located in a common utilities or constants file and used consistently across all UI surfaces to ensure a single source of truth for provider-specific behavior. Update the pendingInputAccent function to use this centralized resolver instead of directly checking membership in PROVIDER_ACCENT_SOURCES.apps/desktop/src/main/services/lanes/laneListSnapshotService.test.ts (1)
51-87: ⚡ Quick winAdd a stale-state regression case for exact pending counts.
The new test only covers
awaitingInputwithpendingInputItemIdpresent. Please add a companion case whereawaitingInput: trueandpendingInputItemIdis missing, and assertpendingInputCount: 0to lock in the exact-actionable contract.🤖 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.test.ts` around lines 51 - 87, The test "counts chat pending input separately from CLI attention heuristics" only covers the case where awaitingInput is true with pendingInputItemId present. Add a companion test case that sets awaitingInput to true in the agentChatService mock but omits the pendingInputItemId (or sets it to undefined), then assert that the resulting snapshot runtime has pendingInputCount: 0 to ensure the exact contract is maintained when pending input ID is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/chat/droidSdkAskUser.ts`:
- Around line 13-16: The filter condition in the filter method only checks if
the option has a length greater than zero, but this allows whitespace-only
strings like " " to pass through and render as blank options downstream.
Modify the filter condition to check that option.trim().length is greater than
zero instead of just option.length, while keeping the map method's label
assignment as is so that meaningful strings are properly preserved with their
trimmed values.
In `@apps/desktop/src/main/services/lanes/laneListSnapshotService.ts`:
- Around line 131-134: The pendingInputCount is being incremented whenever
session.pendingInputWaiting is true, even when session.pendingInputItemId is
null, causing stale pending counts. In the bucket === "awaiting-input"
condition, change the logic so that pendingInputCount is only incremented when
there is an actual actionable pendingInputItemId present, not just when
pendingInputWaiting is true. Replace the condition that currently checks
"session.pendingInputWaiting || session.pendingInputItemId" with a check that
only counts items when pendingInputItemId exists.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 1831-1836: In the handleOption function, the fast-path condition
that checks for submitSingle, non-multiSelect questions, and single question
count does not account for freeform input. When allowsFreeform is true on the
question object, the condition should skip this fast-path to prevent discarding
any draft text the user may have typed in the freeform input. Add an additional
check to the condition to exclude the fast-path when question.allowsFreeform is
true, allowing the normal flow to handle combining the freeform draft text with
the selected option value.
---
Outside diff comments:
In `@apps/desktop/src/main/services/adeActions/registry.test.ts`:
- Around line 955-962: The mocked listSessions function in the test fixture is
missing the pendingInputItemId property in the session object, but the assertion
expects pendingInputCount to be 1. To fix this, either add a concrete
pendingInputItemId value to the mock session object within the listSessions mock
function definition, or update the corresponding assertion to expect
pendingInputCount: 0 to align with a fixture that lacks a pending input item
identifier.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Timeline.swift:
- Around line 111-128: The WorkPlanReviewCard in the .pendingPlanApproval case
is missing the scroll anchor id modifier, which breaks the auto-scroll
functionality for blocking-gate approval cards. Add a .id() modifier to the
WorkPlanReviewCard using the pattern "pending-question-\(plan.id)" to match the
anchor that WorkChatSessionView computes and uses with proxy.scrollTo(),
ensuring the haptic and scroll behavior remains consistent across all plan
approval interactions.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsx`:
- Around line 34-47: The PROVIDER_ACCENT_SOURCES set in the pendingInputAccent
function duplicates provider knowledge that already exists in the
AdeCodeProvider type definition, creating a maintenance risk where providers
could be added to the type but not to this set. Replace the hardcoded
PROVIDER_ACCENT_SOURCES set with a call to a shared provider validation function
(or create one if it doesn't exist) that checks whether a given provider should
receive custom accent styling. This shared function should be located in a
common utilities or constants file and used consistently across all UI surfaces
to ensure a single source of truth for provider-specific behavior. Update the
pendingInputAccent function to use this centralized resolver instead of directly
checking membership in PROVIDER_ACCENT_SOURCES.
In `@apps/desktop/src/main/services/lanes/laneListSnapshotService.test.ts`:
- Around line 51-87: The test "counts chat pending input separately from CLI
attention heuristics" only covers the case where awaitingInput is true with
pendingInputItemId present. Add a companion test case that sets awaitingInput to
true in the agentChatService mock but omits the pendingInputItemId (or sets it
to undefined), then assert that the resulting snapshot runtime has
pendingInputCount: 0 to ensure the exact contract is maintained when pending
input ID is missing.
🪄 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: 37a5ede1-82fb-4044-b584-8661930334ec
⛔ Files ignored due to path filters (2)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**
📒 Files selected for processing (29)
apps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/chat/droidSdkAskUser.test.tsapps/desktop/src/main/services/chat/droidSdkAskUser.tsapps/desktop/src/main/services/chat/droidSdkProtocol.tsapps/desktop/src/main/services/chat/droidSdkWorker.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.test.tsapps/desktop/src/main/services/lanes/laneListSnapshotService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatProposedPlanCard.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/shared/ProviderLogos.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/shared/pendingInputLabels.test.tsapps/desktop/src/shared/pendingInputLabels.tsapps/desktop/src/shared/types/lanes.tsapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADETests/ADETests.swift
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7db76caf6
ℹ️ 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. Already looking forward to the next diff. 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca58d389d6
ℹ️ 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".
| const awaitingUser = sessionNeedsChatTabHighlight({ | ||
| runtimeState: session.runtimeState, | ||
| toolType: session.toolType, | ||
| pendingInputItemId: session.pendingInputItemId, |
There was a problem hiding this comment.
When a chat summary has stale awaitingInput state but no pendingInputItemId (for example after restart, because summaries derive awaitingInput from persisted state even when latestPendingInputItemIdForSession returns null), IPC projects the session as runtimeState: "waiting-input". This new badge then labels that row “Awaiting your input” even though there is no actionable question/plan card to answer; the lane tab logic added here already avoids this by counting only sessions with a pending item id, so this badge should use the same stricter condition.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Greptile Summary
Confidence Score: 4/5
Merge should wait for the pending-input answer preservation issues to be addressed, since both can silently alter user-provided content before it reaches the backend or SDK.
The changed surfaces are broad but the issues are localized to answer shaping paths, with focused evidence confirming the Droid whitespace-loss behavior and the iOS behavior isolated to a specific payload-construction branch.
apps/desktop/src/main/services/chat/agentChatService.ts and apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift
What T-Rex did
Comments Outside Diff (3)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx, line 1831-1834 (link)This branch immediately accepts a single-select answer and sends only the option value. The card still renders an optional freeform field for option questions, so when someone types extra context and then clicks an option, that typed response is silently dropped instead of being included like the
submit()path does. Please either keep this as a selection-only action when freeform is allowed, or include the current draft in the submitted answer.Prompt To Fix With AI
apps/desktop/src/main/services/lanes/laneListSnapshotService.ts, line 201-208 (link)This sets
pendingInputWaiting: truefor any chat summary withawaitingInput, even whenpendingInputItemIdis missing.agentChatServicecan derive that flag from persistedawaitingInput, so after a restart or stale persisted state the lane can show the new “Awaiting you” badge and incrementpendingInputCountwithout an actionable pending card. Since this count is meant to represent exact chat pending inputs, it should only increment when an actual pending input item id is available, or the summary needs a separate field for generic awaiting state.Artifacts
Repro: focused stale pending count script
Stack trace captured during the T-Rex run
Prompt To Fix With AI
apps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swift, line 1185-1191 (link)When a single-choice question also allows freeform text, this branch records only the selected option and then skips the draft text. The caller sends the draft separately as
responseText, but the backend ignoresresponseTextonceanswers[questionId]already exists, so an iOS user who selects an option and types optional context silently loses that context. The option and non-empty freeform text need to be submitted together in the answer value for this question.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "Preserve pending input option answers" | Re-trigger Greptile