Skip to content

Chat 20260514 124455#308

Merged
arul28 merged 3 commits into
mainfrom
ade/chat-20260514-124455-618a1d6d
May 14, 2026
Merged

Chat 20260514 124455#308
arul28 merged 3 commits into
mainfrom
ade/chat-20260514-124455-618a1d6d

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented May 14, 2026

Summary

Describe the change.

What Changed

Key files and behaviors.

Validation

How you tested.

Risks

Anything to watch.

Greptile Summary

This PR delivers several connected improvements across the ADE desktop: lane-aware ADE skill root resolution (all CLI launches and system prompts now use the active lane's worktree path instead of the process cwd), a macOS VM context opt-in guard (replacing the previous over-broad \bvm\b trigger), live transcript recovery polling for grid-tile views, optimistic PTY session rendering with a 2-minute TTL, URL-driven CreatePrModal pre-population from ChatGitToolbar, and promotion of laneDeleteProgressByLaneId from local component state to the global app store.

  • Lane-aware skill roots: getAdeAgentSkillRootsForPrompt({ cwd: laneWorktreePath }) is now threaded through buildTrackedCliLaunchCommand, buildCodingAgentSystemPrompt, agentChatService, and the ADE RPC server, so each lane's worktree is consulted first when resolving bundled agent skills.
  • VM context opt-in: shouldAttachAutomaticMacosVmContext introduces specific regex patterns so buildAutomaticMacosVmContextForPrompt only fires the async getStatus IPC call and injects the context block when the prompt text is clearly about the ADE macOS VM.
  • Grid-tile live updates: A new polling useEffect in AgentChatPane and immediate event flushing for user_message, status, and all visible grid-tile events ensure grid tiles stay current without requiring focus.

Confidence Score: 5/5

Safe to merge; changes are well-tested and improve reliability without breaking existing contracts.

All substantive changes are accompanied by targeted tests and use defensive fallbacks. The two observations are about a polling loop that runs slightly more broadly than its stated intent and a measurement debounce that can skip ticks during rapid non-follow resizes; neither causes data loss or user-visible breakage under normal operation.

AgentChatPane.tsx (recovery polling scope) and AgentChatMessageList.tsx (measurement debounce bail-out) are worth a second look; all other files are straightforward.

Important Files Changed

Filename Overview
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx Adds lane-aware worktree path for CLI launches, live transcript recovery polling for grid tiles, and immediate event flushing for user/status/grid-tile events. Recovery polling is not scoped to grid-tile layout variant.
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx Refactors scroll-to-bottom to support multi-frame follow-up RAF chain; adds new useLayoutEffect for sticky-scroll on height/row changes; revises measurement debounce with a bail-out that may skip measurement ticks during rapid resize when not following.
apps/desktop/src/renderer/lib/visualContextFormatting.ts Adds shouldAttachAutomaticMacosVmContext guard and shortens the injected VM context block; buildAutomaticMacosVmContextForPrompt now accepts promptText/force options.
apps/desktop/src/renderer/components/terminals/useWorkSessions.ts Adds pendingOptimisticSessionsRef with 2-minute TTL so newly launched sessions remain visible across backend refreshes.
apps/desktop/src/renderer/state/appStore.ts Promotes laneDeleteProgressByLaneId from LanesPage local state to global app store with correct reset semantics on project root change.
apps/desktop/src/renderer/components/lanes/LanesPage.tsx Consumes laneDeleteProgressByLaneId from app store; adds queueLaneDeleteRefresh helper and recoverStoredActiveLaneDeletes fallback path.
apps/desktop/src/renderer/components/prs/PRsPage.tsx Adds URL-driven CreatePrModal initial values from query/hash params with a consumed-key ref to prevent double-opening.
apps/desktop/src/renderer/components/prs/CreatePrModal.tsx New initialValues prop pre-populates source lane and target branch; applied idempotently via a ref key.
apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx PR button now navigates to /prs with create=1&sourceLaneId&target=primary params instead of bare /prs when no linked PR is found.
apps/desktop/src/shared/cliLaunch.ts buildTrackedCliLaunchCommand accepts laneWorktreePath and threads it through skill root resolution for all CLI providers.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:3434-3461
**Recovery polling applies to all layout variants, not just grid tiles**

The new `shouldRecoverLiveTranscript` effect fires for every active/awaiting-input session that is `isTileVisible` — which defaults to `true` in non-grid-tile layouts — giving every active focused chat pane a 2.2 s polling loop on top of the existing event-driven updates. The `isTileActive ? 900 : 1200` / `isTileActive ? 2200 : 2800` delay branches suggest the intent is primarily for inactive grid tiles, but nothing prevents the faster 2.2 s branch from running for a regular single-session panel. Consider gating the polling on `layoutVariant === "grid-tile"` (or an equivalent condition) so the periodic `loadHistory({ force: true })` calls don't accumulate in non-grid views.

### Issue 2 of 2
apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx:3958-3972
**Rapid size changes when not following bottom fire only one measurement tick**

The new bail-out path (`if (measureFlushTimer.current) { if (!isFollowingBottom) return; }`) means that when `stickToBottomRef` is false, only the very first size-change notification in a debounce window reschedules the timer — all subsequent ones return early without clearing or rescheduling it. If many rows resize in quick succession while the user has scrolled away, `setMeasurementTick` fires only once and virtualized row heights can be stale until the next user interaction triggers another resize callback outside the debounce window. The original code always rescheduled the timer on each call regardless of the follow state.

Reviews (3): Last reviewed commit: "ship: iteration 1 fix CI and address rev..." | Re-trigger Greptile

@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview May 14, 2026 8:30pm

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Warning

Rate limit exceeded

@arul28 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes and 50 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 847d25cf-d8f3-4ec5-b69b-614e86d04f5c

📥 Commits

Reviewing files that changed from the base of the PR and between 4560bdb and 8bea92f.

⛔ Files ignored due to path filters (12)
  • docs/features/agents/README.md is excluded by !docs/**
  • docs/features/agents/tool-registration.md is excluded by !docs/**
  • docs/features/chat/README.md is excluded by !docs/**
  • docs/features/chat/composer-and-ui.md is excluded by !docs/**
  • docs/features/chat/transcript-and-turns.md is excluded by !docs/**
  • docs/features/computer-use/app-control.md is excluded by !docs/**
  • docs/features/ios-simulator/README.md is excluded by !docs/**
  • docs/features/lanes/README.md is excluded by !docs/**
  • docs/features/pull-requests/README.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/README.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/remote-commands.md is excluded by !docs/**
  • docs/features/terminals-and-sessions/README.md is excluded by !docs/**
📒 Files selected for processing (29)
  • apps/ade-cli/src/adeRpcServer.test.ts
  • apps/ade-cli/src/adeRpcServer.ts
  • apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
  • apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts
  • apps/desktop/src/main/services/ai/tools/systemPrompt.ts
  • apps/desktop/src/main/services/chat/agentChatService.ts
  • apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts
  • apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
  • apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx
  • apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
  • apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx
  • apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx
  • apps/desktop/src/renderer/components/lanes/LanesPage.tsx
  • apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx
  • apps/desktop/src/renderer/components/prs/CreatePrModal.tsx
  • apps/desktop/src/renderer/components/prs/PRsPage.tsx
  • apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts
  • apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
  • apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
  • apps/desktop/src/renderer/lib/chatSessionEvents.test.ts
  • apps/desktop/src/renderer/lib/chatSessionEvents.ts
  • apps/desktop/src/renderer/lib/visualContextFormatting.test.ts
  • apps/desktop/src/renderer/lib/visualContextFormatting.ts
  • apps/desktop/src/renderer/state/appStore.test.ts
  • apps/desktop/src/renderer/state/appStore.ts
  • apps/desktop/src/shared/adeCliGuidance.ts
  • apps/desktop/src/shared/agentSkillRoots.test.ts
  • apps/desktop/src/shared/agentSkillRoots.ts
  • apps/desktop/src/shared/cliLaunch.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade/chat-20260514-124455-618a1d6d

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 14, 2026

@copilot review but do not make fixes

@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 14, 2026

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

@copilot review but do not make fixes

Here's the review of the two commits (8ddfd36 and b96a9f2):


Bulk lane deletion bug fix (8ddfd36)

What it does: Lifts deleteProgressByLaneId out of component-local useState into the global appStore, adds a queueLaneDeleteRefresh helper to batch-add lane IDs before scheduling the debounced refresh, and hardens the hydration effect to recover stored active deletes when the backend listDeleteProgress API is unavailable or fails.

Observations:

  1. appStore clear duplicationlaneDeleteProgressByLaneId: {} is now reset in four separate set() calls inside appStore.ts (lines ~1191, ~1244, ~1332, ~1376 per the diff), plus once more inside setProject on project-root change. That's five clear sites for one slice of state. If a new project-transition code path is added in the future it's easy to forget to add the sixth. Consider consolidating these into a shared resetProjectState partial or at least a comment that the field must be cleared in all five spots.

  2. Double-check the previousProjectRoot !== projectRoot guard in the useEffect — The effect now reads deleteProgressProjectRootRef.current before updating it, and only calls setDeleteProgressByLaneId({}) when the roots differ. However, appStore.setProject already clears laneDeleteProgressByLaneId when the root changes (the new setProject code path), so in the normal case the component effect fires and clears the store a second time redundantly. This is harmless today but worth a note since it means any hydration that runs between setProject and the useEffect cleanup would be wiped out by the duplicate clear.

  3. getStoredActiveLaneIds inside the async .then callback — The hydration effect reads useAppStore.getState().laneDeleteProgressByLaneId directly (not through the deleteProgressByLaneId selector) inside the .then callback. This is intentional for a non-stale snapshot, but it is the only place in the codebase that reaches into useAppStore.getState() from a renderer component. If this pattern spreads it could get confusing. A comment explaining why it bypasses the hook here would be helpful.

  4. setDeleteProgressByLaneId in the useEffect dep array — Adding setDeleteProgressByLaneId to the [project?.rootPath] effect dependency list is correct (it's a stable store action), but the exhaustive-deps lint rule might flag the internal useAppStore.getState() call inside the async closure as missing from the deps. Confirm the linter doesn't require a suppression comment here.

  5. lanesById effect dep — The lanesById effect that prunes stale in-progress entries from deleteProgressByLaneId now lists setDeleteProgressByLaneId as a dependency. Since setDeleteProgressByLaneId is defined with set(...) inside Zustand and is therefore reference-stable across renders, this is fine — it just adds noise to the dep array.


Chat / grid / PR / lane-aware skill roots (b96a9f2)

What it does: Makes agent skill roots lane-worktree-aware across the chat service, CLI launch, the RPC server, and the sync remote command service; reduces the macOS VM automatic-context blob; adds a poll-recovery loop for active grid tiles; makes user_message and status events flush immediately; deep-links "Create PR" from the chat git toolbar to the correct lane and target; and adds an optimistic PTY session entry after launch.

Observations:

  1. shouldAttachAutomaticMacosVmContext regex coverage — The new guard uses \b(ade\s+)?vm\b which will match any prompt containing the word "vm" as a whole word (e.g. "The CI vm failed" or "set vm.args"). This could attach stale macOS VM context to unrelated sends. The more intentional patterns (mac\s*os\s+vm, lane[-\s]?tied, lume) look fine; the bare \bvm\b pattern seems too broad. The test added only confirms the guard doesn't call getStatus for a generic prompt — it doesn't cover a prompt that should not trigger but contains "vm".

  2. activeLaneWorktreePath vs scopedWorktreePathAgentChatPane now has two nearly-identical useMemo hooks: scopedWorktreePath (uses selectedSession?.laneId to handle per-session override) and the new activeLaneWorktreePath (uses laneId prop). The new one is passed only through the CLI launch path. If the session is scoped to a different lane than the pane's laneId, skill roots will diverge between the chat system prompt (which uses the session's lane via agentChatService) and the CLI launch command (which uses activeLaneWorktreePath). This is probably fine given the intended usage, but it's worth documenting the distinction.

  3. Poll-recovery loop in AgentChatPane — The new useEffect that polls loadHistory on an interval while a session is active is guarded by isTileVisible. The interval is cleared on unmount via the cleanup function and the disposed flag is checked in recover(). One edge case: if selectedSession?.status flips from "active" to "idle" mid-interval (e.g. the agent finishes), the effect will re-run and cancel the timer correctly — but there is a window between the last recover() call and the status update where an extra loadHistory could fire for an already-completed session. This is benign but could produce a spurious state update if the component is in the process of unmounting.

  4. scrollFollowFramesRef and useLayoutEffect scroll follow — The new useLayoutEffect fires on containerHeight, groupedRows.length, measurementTick, shouldVirtualize, and totalHeight, calling scrollToBottomSoon(2). scrollToBottomSoon itself is already triggered by the ResizeObserver on the same content size changes, so these two paths can race. The scrollRafRef guard prevents double-scheduling, but the scrollFollowFramesRef.current = Math.max(...) logic means the follow-up frame count will be reset to 2 on every layout update regardless of whether a frame is already in flight. This looks intentionally conservative (always re-arm to at least 2) but could be documented.

  5. buildAdeCliInlineGuidance now delegates to buildAdeCliAgentGuidance — The inline-guidance function body was removed and it just calls buildAdeCliAgentGuidance. The constant ADE_CLI_INLINE_GUIDANCE therefore equals ADE_CLI_AGENT_GUIDANCE. Any caller that imported ADE_CLI_INLINE_GUIDANCE expecting a shorter / different format now gets the full agent guidance. Confirm all callers are OK with the unified text.

  6. agentSkillRoots.ts priority reorder — The cwd-relative paths (worktree-local skills) are now checked before the ADE_AGENT_SKILLS_DIRS_ENV env var and before resourcesPath. This is the intended new precedence (worktree > env > bundled), but flipping the order means a project with a locally committed apps/desktop/resources/agent-skills directory will shadow skills set via ADE_AGENT_SKILLS_DIRS. Make sure that's intentional.

  7. PRsPage URL-based Create PR deep-linkreadCreatePrRouteRequest tries both location.search and window.location.hash. The hash path requires args.hash.startsWith("#/prs") but uses args.hash (not location.hash) so the caller passes window.location.hash directly, bypassing the React Router useLocation hash. In a hash-router setup this is fine, but if the app ever migrates to browser history routing the two sources would diverge. A comment on why window.location.hash is used instead of location.hash from the hook would clarify intent.

  8. Optimistic PTY session in useWorkSessions — The new pendingOptimisticSessionsRef map is cleared on project change (in the reset effect) but is not cleared when the Work tab becomes inactive (active = false). Sessions in the map created during an inactive period would be replayed on reactivation. The 2-minute TTL prevents this from being a persistent issue, but the map could grow without bound during a long inactive session list if launches keep failing.


Tests: New tests cover the immediate flush for user_message, the grid-tile live streaming path, the poll-recovery loop, the "no macOS VM context for unrelated sends" guard, and the store-level lane delete progress behavior. These are well-targeted.

No blocking issues — mostly documentation and a couple of potential edge cases noted above.

Copilot finished work on behalf of arul28 May 14, 2026 19:33
Comment thread apps/desktop/src/renderer/lib/visualContextFormatting.ts
Comment thread apps/desktop/src/shared/agentSkillRoots.ts
@arul28 arul28 force-pushed the ade/chat-20260514-124455-618a1d6d branch from b96a9f2 to ee2e220 Compare May 14, 2026 20:08
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 14, 2026

@copilot review but do not make fixes

@arul28 arul28 force-pushed the ade/chat-20260514-124455-618a1d6d branch from ee2e220 to 8bea92f Compare May 14, 2026 20:30
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 14, 2026

@copilot review but do not make fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants