Cli Fixes#330
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds configurable startup delay for PTY terminal sessions with lifecycle safeguards, propagates the delay parameter through renderer callbacks, and improves optimistic session reconciliation when persisted rows are refreshed from the server. ChangesStartup delay and session optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
@copilot review but do not make fixes |
|
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. |
arul28
force-pushed
the
ade/cli-fixes-87ec38d0
branch
from
0f28f6c to
511b811
Compare
|
@copilot review but do not make fixes |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 12 file(s), +219 / −30
Verdict: Looks good
This PR fixes Work-tab CLI launch races: it defers typing startupCommand into the shell (180 ms, clamped in main), routes Work chat launches through the shell + startupCommand path instead of direct argv, and merges optimistic PTY ids into persisted session rows when a refresh lands before ptyId is backfilled. The changes are focused, tested, and align with the terminals docs.
Notes
- Optimistic merge (
mergePendingOptimisticSessioninuseWorkSessions.ts) correctly targets the race wherelistSessionsreturns a running row with a null/mismatchedptyIdwhile the live create result is already known;keepPendinguntil ids match is the right follow-up behavior. - Startup delay is bounded in main (
normalizeStartupCommandDelayMs, max 1000 ms), guarded on dispose before write, and covered by a fake-timer test inptyService.test.ts. - Work CLI path intentionally drops
command/argsso launches always go through shell + delayedstartupCommand, which matches the documented fix for half-rendered scrollback and shell-shim resolution; otherlaunchPtySessioncallers can still passcommand/argsand omitstartupDelayMs. - Not verified in this run: full desktop test suite or manual Work-tab launch on macOS; review is based on diff + targeted file reads.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Reviewed commit |
|
@copilot review but do not make fixes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
155-155: ⚡ Quick winRename this constant to camelCase for guideline compliance.
Line 155 introduces a new constant using SCREAMING_SNAKE_CASE; this file should use camelCase variable names per project rule.
♻️ Proposed rename
-const WORK_CLI_STARTUP_DELAY_MS = 180; +const workCliStartupDelayMs = 180;- startupDelayMs: WORK_CLI_STARTUP_DELAY_MS, + startupDelayMs: workCliStartupDelayMs,As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.🤖 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/renderer/components/chat/AgentChatPane.tsx` at line 155, Rename the constant WORK_CLI_STARTUP_DELAY_MS to camelCase (e.g., workCliStartupDelayMs) throughout AgentChatPane.tsx: update its declaration and every usage site (references in functions, JSX, hooks) so identifiers match, and run a quick type-check to ensure no remaining references to the old name remain; keep the numeric value (180) unchanged.
🤖 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/renderer/components/chat/AgentChatPane.tsx`:
- Line 155: Rename the constant WORK_CLI_STARTUP_DELAY_MS to camelCase (e.g.,
workCliStartupDelayMs) throughout AgentChatPane.tsx: update its declaration and
every usage site (references in functions, JSX, hooks) so identifiers match, and
run a quick type-check to ensure no remaining references to the old name remain;
keep the numeric value (180) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5db3842e-a524-4455-9787-ac312e62b760
⛔ Files ignored due to path filters (3)
docs/features/chat/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (9)
apps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/types/sessions.ts
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade/cli-fixes-87ec38d0
branch
from
e226b29 to
d127917
Compare
|
@copilot review but do not make fixes |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Tests
Greptile Summary
This PR completes the fix for the
startupTimerhandle not being stored on the PTY entry: the handle is now persisted onentry.startupTimer, cleared in bothcloseEntryanddispose, and self-nulled inside the callback — exactly matching the existingaiTitleTimerpattern. A new test covers the dispose-during-delay scenario, and docs are updated to reflect the delay clamping,unref, and Work-CLI use case.ptyService.ts:PtyEntrygains astartupTimerfield; bothcloseEntry(natural exit) anddispose(explicit kill) now clear the timer before the asyncpty.kill()completes, preventing stale closures from firing after PTY teardown.ptyService.test.ts: New test creates a PTY with a 180 ms startup delay, disposes it immediately, advances fake timers to 180 ms, and assertspty.writewas never called — directly validating the fix.AgentChatPane.tsx: Module-level constant renamed fromWORK_CLI_STARTUP_DELAY_MStoworkCliStartupDelayMs(camelCase); no behavioral change.Confidence Score: 5/5
Safe to merge — the timer lifecycle is now correctly managed across both natural-exit and explicit-dispose paths, with a test validating the fixed race condition.
The change is targeted and well-scoped: the timer handle is stored, cleared in every teardown path, and the callback guards with an entry.disposed check as belt-and-suspenders. The new test directly exercises the previously broken case. No behavioral regressions are expected.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant ptyService participant entry as PtyEntry participant timer as setTimeout participant pty as node-pty Caller->>ptyService: "create({ startupCommand, startupDelayMs: 180 })" ptyService->>entry: "init (startupTimer = null)" ptyService->>timer: setTimeout(writeStartupCommand, 180) ptyService->>entry: "entry.startupTimer = handle" ptyService-->>Caller: "{ ptyId, sessionId }" alt dispose() called before timer fires Caller->>ptyService: "dispose({ ptyId, sessionId })" ptyService->>entry: "entry.disposed = true" ptyService->>timer: clearTimeout(entry.startupTimer) ptyService->>entry: "entry.startupTimer = null" ptyService->>pty: pty.kill() Note over timer: timer cancelled — write never happens else timer fires normally timer->>ptyService: writeStartupCommand() ptyService->>entry: "entry.startupTimer = null" ptyService->>entry: check entry.disposed (false) ptyService->>pty: pty.write(command) endReviews (4): Last reviewed commit: "ship: iteration 2 - address coderabbit n..." | Re-trigger Greptile