Fix Work attention for SDK chat questions - #895
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesChat Session Projection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/sessions/chatSessionProjection.ts (1)
128-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
listSessionsWithChatProjectionduplicates (and slightly diverges from) theIPC.sessionsListfetch pipeline.This helper re-implements the fetch/enrich/chat-fetch flow that
IPC.sessionsList(registerIpc.ts lines 6363-6403) already has, but omits the resume-target hydration step (sessionNeedsResumeTargetHydration+ptyService.ensureResumeTargets) that the IPC handler performs before enrichment. Sessions read through the adeActionssession.listdomain (this helper) can therefore lack resume metadata that the desktopsessionsListIPC call would have hydrated. Consider factoring the shared fetch/enrich steps (including resume hydration) into this module so both call sites stay in sync going forward.🤖 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/sessions/chatSessionProjection.ts` around lines 128 - 152, Update listSessionsWithChatProjection to share the session fetch/enrichment pipeline with IPC.sessionsList, including sessionNeedsResumeTargetHydration and ptyService.ensureResumeTargets before enrichment. Factor the common flow into a reusable helper in this module and have both call sites use it, preserving the existing chat projection behavior and keeping both paths synchronized.
🤖 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/sessions/chatSessionProjection.ts`:
- Around line 128-152: Update listSessionsWithChatProjection to share the
session fetch/enrichment pipeline with IPC.sessionsList, including
sessionNeedsResumeTargetHydration and ptyService.ensureResumeTargets before
enrichment. Factor the common flow into a reusable helper in this module and
have both call sites use it, preserving the existing chat projection behavior
and keeping both paths synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05b9ac0a-c6d6-47a9-bb1a-f065890c810a
⛔ Files ignored due to path filters (1)
docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (4)
apps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/sessions/chatSessionProjection.ts
Summary
Verification
Summary by CodeRabbit
Greptile Summary
This PR moves chat session state projection into a shared path for IPC and ADE runtime reads. The main changes are:
session.listandsession.getnow use the same projection path as IPC.Confidence Score: 5/5
Safe to merge with low risk.
The updated code moves existing IPC behavior into a shared helper and extends it to runtime actions, with tests for the main state transitions and failure paths. No blocking correctness or security issues were found in the changed paths.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
Reviews (2): Last reviewed commit: "fix: share session resume hydration pipe..." | Re-trigger Greptile