PRs Tab Fixes#240
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds session-level blocking for unresolved pending user inputs across chat service, desktop composer/pane, and iOS UI; updates lane PR selection/display to surface merged PRs when head branch matches and renders per-lane PR badges; enriches PR convergence inventory with expanded review-thread context and improves CI/check-run deduplication. ChangesPending-input blocking (chat service → UI wiring → tests)
Lane PR display & badges (service → helpers → UI → tests)
PR detail convergence & checks (data mapping → unified checks → UI/tests)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Here's a code review of the PR changes.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 737-740: Re-check the composer lock and parallel-attachment limits
inside the attachment ingestion paths (e.g., addFileAttachments and the other
attachment handler near the later block) instead of relying only on the UI entry
guard; specifically, inside addFileAttachments (and the analogous handler
referenced in the comment) immediately bail out if composerInputLocked is true
and surface composerInputLockMessage (or the default "Resolve the pending
request before adding attachments.") as the rejection reason, and also re-check
parallelChatMode && attachments.length >= PARALLEL_CHAT_MAX_ATTACHMENTS to
prevent adding files past the PARALLEL_CHAT_MAX_ATTACHMENTS limit. Ensure you
update any success/error flows to respect these checks so files selected before
the lock do not get appended after lock activation.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 3715-3723: The submit flow currently clears promptSuggestion too
early (onSubmit) which causes suggestions to be lost when the submit is blocked;
update the logic so promptSuggestion is only cleared after the pending-input
guard passes and a submit is actually initiated: in the submit handler (submit)
and/or onSubmit caller, move the setPromptSuggestion("") (or the prompt
suggestion clearing logic) to after the checks that reference submitInFlightRef,
busy, parallelLaunchBusy, selectedSessionId,
pendingInputsBySession[selectedSessionId], and selectedSession?.awaitingInput,
and ensure it runs only when you proceed to send (i.e., after the guard returns
false and before/after the actual send logic as appropriate).
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 669-676: The async updater refreshLanePrTags currently sets
setLanePrTags unconditionally after awaiting window.ade.prs.listAll(), allowing
stale responses to overwrite state when project context changes; fix by adding a
cancellation token or requestId check (e.g., a useRef requestId or isMounted
ref) captured when refreshLanePrTags starts and validated before calling
setLanePrTags, and apply the same pattern to the other similar functions at the
referenced ranges; ensure the token/requestId is incremented or cleared on
context change or unmount so only the latest response updates state.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift`:
- Around line 148-149: The pending-input warning text is shown when
hasPendingInputGate || sessionStatus == "awaiting-input" but the composer
tint/background still only checks sessionStatus == "awaiting-input"; update the
composer styling checks (the view that sets the tint/background in
WorkChatSessionView/composer UI) to use the same combined condition
(hasPendingInputGate || sessionStatus == "awaiting-input") so the visual state
matches the blocking warning message.
🪄 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: a234f7b1-e6ae-43e9-bb4e-90b8da2abfdc
⛔ Files ignored due to path filters (4)
apps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (17)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsxapps/desktop/src/renderer/components/prs/shared/prFormatters.tsapps/ios/ADE/Views/Work/WorkChatSessionView.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…nding-input lock - prs: lane-scoped PR display falls back to merged/closed PRs; state-aware badge labels (`formatPrBadgeLabel`) - prs: PrConvergencePanel issue rows are expandable with author/body/thread comment count, "show ignored" toggle, "Ignore comment" rename, manual-run round labels - prs: dedupe Actions check entries; ignore/expand review items - lanes: state-aware lane-list PR tag via `selectLanePrTag` - chat: hard-lock composer + server-side gate while `pendingInput.blocking` is set; merged-PR display in PrDetailPane - ios: WorkChatSessionView updates to match new chat lock contract - docs: refresh chat/composer-and-ui, lanes/README, pull-requests/README
arul28
force-pushed
the
ade/prs-tab-fixes-3c147957
branch
from
ad99a52 to
cd92904
Compare
|
@copilot review but do not make fixes |
Here's a review of the changes, incorporating the Greptile findings now visible in the PR description. 🔴 TypeScript type error —
|
- AgentChatPane: defer promptSuggestion clear until after pending-input guard so blocked submits don't lose the suggestion (CodeRabbit PRRT_kwDORNN9Fc5_OIgJ) - LanesPage: guard refreshLanePrTags with requestId + project-root capture so stale listAll() responses don't overwrite state (CodeRabbit PRRT_kwDORNN9Fc5_OIgL) - LanesPage: drop .toLowerCase() from normalizeLanePrBranch so client matching mirrors server-side normalizeBranchName (Copilot) - prService: inline rowMatchesCurrentLaneBranch dead indirection at its 2 call sites (Copilot) - PrConvergencePanel: render threadLatestCommentAt as "(updated <time>)"; cap ready-state round label at maxRounds; apply visibleReviewCommentItems filter in terminal-state branch (Copilot) - WorkChatSessionView.swift: align awaitingInputGate initializer indentation (Copilot)
|
@copilot review but do not make fixes |
Here's a review incorporating the new Greptile findings from the second pass. 🟡 Submit guard vs. composer lock inconsistency —
|
- AgentChatPane.submit: only block on pending inputs whose request.blocking is true (matches AgentChatComposer's hard-lock condition); non-blocking pending inputs no longer cause silent send failures (Copilot) - prService.getDisplayRowForCurrentLaneBranch / getActiveRowForCurrentLaneBranch: return null for branch-less lanes (lane.branch_ref empty) instead of falling back to rows[0]; restores prior contract so unbranched lanes never claim a stray PR badge (Copilot) - prService.test: assert null for branch-less lane case
|
@copilot review but do not make fixes |
Here's a review of the latest state of the PR after the two shipped iterations. 🟡
|
Terminal-state branch of PrConvergencePanel respected showIgnoredItems but provided no toggle button to flip it, so dismissed items on merged/closed PRs were silently hidden with no way to reveal them. Surface the toggle next to the "Historical review inventory" heading and render the section whenever there are visible OR dismissed items, mirroring the active-state toolbar button (Copilot/Greptile P1).
|
@copilot review but do not make fixes |
Here's a review of the PR after iter 3. ✅ All previously-flagged P1 / P2 issues are now resolved
🟡 One minor nit remaining
SummaryThe PR is in a clean, shippable state. The Greptile P2 regression (dismissed items silently hidden in terminal state) is fully addressed in iter 3, and all carry-forward findings from the earlier reviews are resolved. The one note above ( |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
2739-2750:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe steer lock is still incomplete.
This hides the draft steer button, but the staged-message strip later in the component still keeps
onSendNow/onInterruptactive. With queued steers present, a blocking pending card can still be bypassed from that list.Suggested fix
- {pendingSteers.map((steer) => ( + {pendingSteers.map((steer) => ( <PendingSteerItem key={steer.steerId} steer={steer} onCancel={() => onCancelSteer?.(steer.steerId)} onEdit={(text) => onEditSteer?.(steer.steerId, text)} - onSendNow={onDispatchSteerInline ? () => onDispatchSteerInline(steer.steerId) : undefined} - onInterrupt={onDispatchSteerInterrupt ? () => onDispatchSteerInterrupt(steer.steerId) : undefined} + onSendNow={!composerInputLocked && onDispatchSteerInline ? () => onDispatchSteerInline(steer.steerId) : undefined} + onInterrupt={!composerInputLocked && onDispatchSteerInterrupt ? () => onDispatchSteerInterrupt(steer.steerId) : undefined} /> ))}If you want the lock to be fully consistent, pass a
disabledprop intoPendingSteerItemand use it for edit/remove there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 2739 - 2750, The draft-steer button is hidden when composerInputLocked is true but the staged-message strip still allows bypass via PendingSteerItem handlers; update the staged-message rendering to pass a disabled prop (e.g., disabled={composerInputLocked}) into PendingSteerItem and ensure PendingSteerItem uses that prop to disable edit/remove and to no-op or block calls to onSendNow and onInterrupt; locate the references to submitComposerDraft, composerInputLocked, and PendingSteerItem in the component and wire the disabled flag through so queued steers cannot be acted on while locked.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (1)
828-834:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalidate in-flight fetches when handling
prs-updatedevents.At Line 830-833,
setLanePrTags(event.prs)does not bumplanePrTagsRequestRef. An older in-flightrefreshLanePrTags()can still resolve later and overwrite this newer event payload.💡 Proposed fix
useEffect(() => { return window.ade.prs.onEvent((event) => { if (event.type === "prs-updated") { + lanePrTagsRequestRef.current += 1; setLanePrTags(event.prs); } else if (event.type === "pr-notification") { + lanePrTagsRequestRef.current += 1; void refreshLanePrTags(); } }); }, [refreshLanePrTags]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 828 - 834, When handling the "prs-updated" event in the window.ade.prs.onEvent callback, also bump/invalidate the in-flight fetch token so an older refreshLanePrTags() resolution cannot overwrite the new event data; update the same request token/ref used by refreshLanePrTags (lanePrTagsRequestRef) inside the event.type === "prs-updated" branch immediately before/after calling setLanePrTags(event.prs) (e.g., increment or set to a new unique value) so refreshLanePrTags can ignore stale responses by comparing its local token to lanePrTagsRequestRef.apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
737-740:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUI-only attachment locking still has a bypass.
canAttachdisables the visible entry points, but the async ingestion paths can still append files if the native picker or clipboard temp-save started beforependingInput.blockingflipped. Please re-check the lock insideaddFileAttachmentsandaddNativeClipboardImageAttachmentbefore eachonAddAttachment, and surfacecomposerInputLockMessagewhen rejecting.Suggested fix
const addFileAttachments = async (files: FileList | null | undefined) => { if (!files?.length) return; + if (composerInputLocked) { + setAttachError(composerInputLockMessage ?? "Resolve the pending request before adding attachments."); + return; + } if (parallelChatMode && attachments.length >= PARALLEL_CHAT_MAX_ATTACHMENTS) return; if (fileAddInProgressRef.current) return; fileAddInProgressRef.current = true; setAttachError(null); try { let addedInBatch = 0; for (const file of Array.from(files)) { + if (composerInputLocked) { + setAttachError(composerInputLockMessage ?? "Resolve the pending request before adding attachments."); + break; + } ... if (hasRealPath) { + if (composerInputLocked) break; onAddAttachment({ path: filePath, type: inferAttachmentType(filePath, file.type) }); addedInBatch += 1; continue; } ... const { path: tempPath } = await window.ade.agentChat.saveTempAttachment(...); + if (composerInputLocked) { + setAttachError(composerInputLockMessage ?? "Resolve the pending request before adding attachments."); + break; + } onAddAttachment({ path: tempPath, type: inferAttachmentType(tempPath, file.type) }); } } finally { fileAddInProgressRef.current = false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 737 - 740, The UI lock check (canAttach/attachBlockedReason) is insufficient because async ingestion can start before pendingInput.blocking flips; update addFileAttachments and addNativeClipboardImageAttachment to re-check composerInputLocked (and parallel attachment limit via attachments.length < PARALLEL_CHAT_MAX_ATTACHMENTS) immediately before invoking onAddAttachment, and if blocked return/reject early while surfacing composerInputLockMessage (or the parallel-limit message) as the rejection reason instead of proceeding. Ensure you reference composerInputLockMessage when rejecting and keep the existing onAddAttachment call-site unchanged except for this pre-check.
🤖 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/prs/prService.ts`:
- Around line 5239-5240: Validation failures are due to TypeScript config and
missing dev dependencies, not the getDisplayRowForCurrentLaneBranch/rowToSummary
logic; fix by adding `@types/node` to devDependencies and/or include "types":
["node"] in tsconfig's compilerOptions so TS recognizes Node types, remove or
update any deprecated tsconfig option (the reported deprecated baseUrl) to the
supported configuration, and install the missing build/lint tools (add tsup and
eslint to devDependencies and ensure eslint plugin modules used by the project
are installed) so typecheck, build, and lint succeed before approving changes
that reference getDisplayRowForCurrentLaneBranch and rowToSummary.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 2739-2750: The draft-steer button is hidden when
composerInputLocked is true but the staged-message strip still allows bypass via
PendingSteerItem handlers; update the staged-message rendering to pass a
disabled prop (e.g., disabled={composerInputLocked}) into PendingSteerItem and
ensure PendingSteerItem uses that prop to disable edit/remove and to no-op or
block calls to onSendNow and onInterrupt; locate the references to
submitComposerDraft, composerInputLocked, and PendingSteerItem in the component
and wire the disabled flag through so queued steers cannot be acted on while
locked.
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 737-740: The UI lock check (canAttach/attachBlockedReason) is
insufficient because async ingestion can start before pendingInput.blocking
flips; update addFileAttachments and addNativeClipboardImageAttachment to
re-check composerInputLocked (and parallel attachment limit via
attachments.length < PARALLEL_CHAT_MAX_ATTACHMENTS) immediately before invoking
onAddAttachment, and if blocked return/reject early while surfacing
composerInputLockMessage (or the parallel-limit message) as the rejection reason
instead of proceeding. Ensure you reference composerInputLockMessage when
rejecting and keep the existing onAddAttachment call-site unchanged except for
this pre-check.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 828-834: When handling the "prs-updated" event in the
window.ade.prs.onEvent callback, also bump/invalidate the in-flight fetch token
so an older refreshLanePrTags() resolution cannot overwrite the new event data;
update the same request token/ref used by refreshLanePrTags
(lanePrTagsRequestRef) inside the event.type === "prs-updated" branch
immediately before/after calling setLanePrTags(event.prs) (e.g., increment or
set to a new unique value) so refreshLanePrTags can ignore stale responses by
comparing its local token to lanePrTagsRequestRef.
🪄 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: 0b12b9f0-95bb-4edf-b064-935a9ab82e9c
⛔ Files ignored due to path filters (4)
apps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (17)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsxapps/desktop/src/renderer/components/prs/shared/prFormatters.tsapps/ios/ADE/Views/Work/WorkChatSessionView.swift
✅ Files skipped from review due to trivial changes (4)
- apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx
- apps/ios/ADE/Views/Work/WorkChatSessionView.swift
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/desktop/src/renderer/components/lanes/LanesPage.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx
- apps/desktop/src/main/services/prs/prService.test.ts
- apps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsx
- apps/desktop/src/main/services/chat/agentChatService.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx
- apps/desktop/src/renderer/components/prs/shared/prFormatters.ts
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Bug Fixes
New Features
Greptile Summary
This PR fixes several PRs-tab and chat-composer regressions: the chat composer now locks while a blocking pending-input card is active (desktop + iOS), merged PRs matching the current lane branch are surfaced in the sidebar badge and chat toolbar, GitHub Actions check-run deduplication is improved by job ID and unique-name matching, and the Convergence panel gains expandable comment context with an ignored-items toggle.
Confidence Score: 5/5
Safe to merge; all findings are P2 edge cases with no current data-loss or breakage risk.
No P0/P1 issues found. The two P2 comments cover a narrow Electron IPC race during project switch (prs-updated guard) and a clarification question about whether non-blocking freeform questions still exist in practice. Both are low-probability in a desktop app and do not affect the primary user paths changed by this PR.
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (prs-updated event guard) and apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (non-blocking question submit removal)
Important Files Changed
Sequence Diagram
sequenceDiagram participant Renderer as LanesPage / ChatPane participant PRSvc as prService (main) participant DB as SQLite Note over Renderer,DB: Lane PR badge (sidebar) Renderer->>PRSvc: window.ade.prs.listAll() PRSvc->>DB: SELECT all PRs (lane_id, project_id) DB-->>PRSvc: PullRequestRow[] PRSvc-->>Renderer: PrSummary[] (all states incl. merged) Renderer->>Renderer: selectLanePrTag(lane, prs)<br/>prefer open/draft > merged > closed Note over Renderer,DB: Chat toolbar badge Renderer->>PRSvc: prs.getForLane(laneId) PRSvc->>DB: SELECT ORDER BY state priority + recency DB-->>PRSvc: PullRequestRow[] PRSvc->>PRSvc: rowMatchesLaneBranchForDisplay()<br/>(branch match, no state filter) PRSvc-->>Renderer: PrSummary | null (merged OK) Note over Renderer,DB: Live PR event update PRSvc-->>Renderer: onEvent(prs-updated, prs) Renderer->>Renderer: setLanePrTags(prs) no project guard Note over Renderer,DB: Pending input gate Renderer->>Renderer: composerInputLocked = pendingInput?.blocking Renderer->>PRSvc: sendMessage() blocked if hasLivePendingInputPrompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: iter 3 — Show ignored toggle reach..." | Re-trigger Greptile