fix(desktop): pty resume backfill + persist per-project route#199
fix(desktop): pty resume backfill + persist per-project route#199
Conversation
…ect route
- ptyService: when relaunching a tracked CLI session whose resumeMetadata
has no targetId, run tryBackfillResumeTarget("resume-launch") before
spawning so the new pty starts on the right resume command instead of
cold-starting. Replaces the old warn-only branch.
- AppShell: persist the last-visited route per project to localStorage
and restore it on project switch, instead of always slamming /work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThree interconnected changes: a new unit test for resuming tracked Claude sessions with resume command backfilling, implementation of resume target backfill logic during PTY creation to update session metadata when missing target IDs, and addition of localStorage-based per-project "last visited" route persistence in the AppShell component with fallback to "/work". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const allowedRoots = [ | ||
| "/project", | ||
| "/lanes", | ||
| "/files", | ||
| "/work", | ||
| "/graph", | ||
| "/prs", | ||
| "/review", | ||
| "/history", | ||
| "/automations", | ||
| "/missions", | ||
| "/cto", | ||
| "/settings", | ||
| ]; | ||
| if (!allowedRoots.some((root) => pathname === root || pathname.startsWith(`${root}/`))) { | ||
| return null; | ||
| } | ||
| return route; |
There was a problem hiding this comment.
allowedRoots diverges from primaryTabPath roots
serializeLocationRoute adds /review and /cto to its allow-list, but primaryTabPath (defined just above) does not include them. Routes under those paths will therefore be persisted and later restored, but the sidebar tab won't be activated (since primaryTabPath won't match them). If /review and /cto are real top-level pages that should be restored, they should also be added to primaryTabPath; otherwise they should be dropped from allowedRoots.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/app/AppShell.tsx
Line: 75-92
Comment:
**`allowedRoots` diverges from `primaryTabPath` roots**
`serializeLocationRoute` adds `/review` and `/cto` to its allow-list, but `primaryTabPath` (defined just above) does not include them. Routes under those paths will therefore be persisted and later restored, but the sidebar tab won't be activated (since `primaryTabPath` won't match them). If `/review` and `/cto` are real top-level pages that should be restored, they should also be added to `primaryTabPath`; otherwise they should be dropped from `allowedRoots`.
How can I resolve this? If you propose a fix, please make it concise.
| const shouldBackfillResumeTarget = | ||
| existingSession | ||
| && isTrackedCliToolType(toolTypeHint) | ||
| && !existingSession.resumeMetadata?.targetId?.trim(); | ||
| if (shouldBackfillResumeTarget) { | ||
| const backfilled = await tryBackfillResumeTarget(sessionId, toolTypeHint, "resume-launch", cwd); | ||
| const updatedSession = backfilled ? sessionService.get(sessionId) : null; | ||
| if (updatedSession?.resumeCommand?.trim()) { | ||
| initialResumeCommand = updatedSession.resumeCommand.trim(); | ||
| initialResumeMetadata = updatedSession.resumeMetadata ?? initialResumeMetadata; | ||
| startupCommand = initialResumeCommand; | ||
| } | ||
| } |
There was a problem hiding this comment.
When tryBackfillResumeTarget succeeds, startupCommand is overwritten with the backfilled command (e.g. claude --resume <id>). However, the original requestedStartupCommand may contain flags such as --permission-mode default that are not preserved in the backfilled command. The PTY therefore launches without those flags.
This may be intentional if permission mode is meant to come from session metadata at the tool level, but the test's expected invocation ("claude --resume claude-session-123\r") silently drops --permission-mode default that was in the startup command. Worth verifying the tool CLI picks up the correct permission mode without the explicit flag on resume.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/pty/ptyService.ts
Line: 1160-1172
Comment:
**Backfilled command drops original launch flags**
When `tryBackfillResumeTarget` succeeds, `startupCommand` is overwritten with the backfilled command (e.g. `claude --resume <id>`). However, the original `requestedStartupCommand` may contain flags such as `--permission-mode default` that are not preserved in the backfilled command. The PTY therefore launches without those flags.
This may be intentional if permission mode is meant to come from session metadata at the tool level, but the test's expected invocation (`"claude --resume claude-session-123\r"`) silently drops `--permission-mode default` that was in the startup command. Worth verifying the tool CLI picks up the correct permission mode without the explicit flag on resume.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Bug fix bundled into v1.1.5 release.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR fixes two independent bugs: (1) the PTY service now actively backfills the resume target (session ID) before spawning the PTY instead of only logging a warning, and (2)
AppShellpersists the last-visited route per project tolocalStorageand restores it on project switch.Confidence Score: 4/5
Safe to merge; only P2 findings present.
Both fixes are well-scoped and backed by tests. The two P2 findings (allowedRoots inconsistency and potential flag-dropping on backfill) don't cause regressions but are worth addressing in a follow-up.
apps/desktop/src/renderer/components/app/AppShell.tsx – verify /review and /cto route handling; apps/desktop/src/main/services/pty/ptyService.ts – confirm --permission-mode is not needed in the backfilled command.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant PtyService participant SessionService participant tryBackfillResumeTarget Caller->>PtyService: create({ sessionId, startupCommand: "claude --resume" }) PtyService->>SessionService: get(sessionId) → existingSession alt existingSession && no targetId PtyService->>tryBackfillResumeTarget: await tryBackfillResumeTarget("resume-launch") tryBackfillResumeTarget->>SessionService: readTranscriptTail() tryBackfillResumeTarget->>SessionService: setResumeCommand(sessionId, "claude --resume <id>") tryBackfillResumeTarget-->>PtyService: true PtyService->>SessionService: get(sessionId) → updatedSession PtyService->>PtyService: startupCommand = "claude --resume <id>" end PtyService->>PtyService: spawn shell PTY PtyService->>PtyService: pty.write("claude --resume <id>\r")Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(desktop): backfill pty resume target..." | Re-trigger Greptile