Track per-thread terminal running state in UI and store#19
Track per-thread terminal running state in UI and store#19juliusmarminge merged 5 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds terminal lifecycle tracking: threads gain Changes
Sequence DiagramsequenceDiagram
participant Terminal as Terminal
participant Server as Server/TerminalManager
participant App as App (web)
participant Store as Redux Store
participant UI as Sidebar/ChatView
Terminal->>Server: emits low-level process events / subprocess state
Server->>App: sends terminal events (started/activity/exited)
App->>App: effect receives event
App->>Store: dispatch APPLY_TERMINAL_EVENT { event }
Store->>Store: update thread.runningTerminalIds based on event
Store->>UI: state update triggers re-render
UI->>UI: render Terminal status pill or update ChatView terminal state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Track per-thread terminal subprocess state and emit
|
Greptile OverviewGreptile SummaryAdded Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatView
participant ThreadTerminalDrawer
participant Store
participant App
participant API
Note over User,API: Terminal Open Flow
User->>ChatView: Open terminal
ChatView->>ThreadTerminalDrawer: Render drawer
ThreadTerminalDrawer->>API: terminal.open(threadId, cwd, cols, rows)
API-->>ThreadTerminalDrawer: snapshot (status: "running")
ThreadTerminalDrawer->>Store: onRunningStateChange(true)
Store->>Store: SET_THREAD_TERMINAL_RUNNING
Note over User,API: Terminal Event Flow (started/restarted)
API->>App: terminal.onEvent({type: "started", snapshot})
App->>Store: APPLY_TERMINAL_EVENT
Store->>Store: Update terminalRunning based on snapshot.status
API->>ThreadTerminalDrawer: terminal.onEvent({type: "started"})
ThreadTerminalDrawer->>ThreadTerminalDrawer: Clear terminal, write history
Note over User,API: Terminal Event Flow (exited)
API->>App: terminal.onEvent({type: "exited"})
App->>Store: APPLY_TERMINAL_EVENT
Store->>Store: Set terminalRunning=false, terminalOpen=false
API->>ThreadTerminalDrawer: terminal.onEvent({type: "exited"})
ThreadTerminalDrawer->>Store: onThreadExited()
Note over User,API: Terminal Event Flow (error)
API->>App: terminal.onEvent({type: "error"})
App->>Store: APPLY_TERMINAL_EVENT
Store->>Store: Set terminalRunning=false
API->>ThreadTerminalDrawer: terminal.onEvent({type: "error"})
ThreadTerminalDrawer->>Store: onRunningStateChange(false)
ThreadTerminalDrawer->>ThreadTerminalDrawer: Write error message
Note over User,API: UI Update
Store-->>ChatView: State update
ChatView-->>User: Terminal status pill visible in sidebar
Last reviewed commit: 69a04d1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| terminalRunning: | ||
| action.event.type === "started" || action.event.type === "restarted" | ||
| ? action.event.snapshot.status === "running" | ||
| : action.event.type === "exited" || action.event.type === "error" | ||
| ? false | ||
| : t.terminalRunning, |
There was a problem hiding this comment.
Status check could be more robust. While started events currently always have status: "running" (terminal sets "starting" → spawns PTY → sets "running" → emits event), consider checking for "starting" as well or setting terminalRunning: true unconditionally for these event types. This would make the code more resilient to future changes in the terminal manager.
| terminalRunning: | |
| action.event.type === "started" || action.event.type === "restarted" | |
| ? action.event.snapshot.status === "running" | |
| : action.event.type === "exited" || action.event.type === "error" | |
| ? false | |
| : t.terminalRunning, | |
| terminalRunning: | |
| action.event.type === "started" || action.event.type === "restarted" | |
| ? action.event.snapshot.status === "running" || action.event.snapshot.status === "starting" | |
| : action.event.type === "exited" || action.event.type === "error" | |
| ? false | |
| : t.terminalRunning, |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
juliusmarminge
force-pushed
the
codething/696df84f
branch
from
01bf487 to
cf1dce9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/store.test.ts`:
- Around line 79-109: The type error is caused because overrides:
Partial<TerminalEvent> allows changing the discriminant "type" and yields an
incompatible union; update both makeTerminalStartedEvent and
makeTerminalActivityEvent to accept overrides: Partial<Omit<TerminalEvent,
"type">> (i.e., exclude the "type" field from the override) so the functions
keep their fixed discriminant ("started" or "activity") while still allowing
other fields to be overridden; keep the return type as TerminalEvent and spread
overrides as before.
| function makeTerminalStartedEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent { | ||
| return { | ||
| type: "started", | ||
| threadId: "thread-local-1", | ||
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | ||
| createdAt: "2026-02-09T00:00:01.000Z", | ||
| snapshot: { | ||
| threadId: "thread-local-1", | ||
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | ||
| cwd: "/tmp/project", | ||
| status: "running", | ||
| pid: 1234, | ||
| history: "", | ||
| exitCode: null, | ||
| exitSignal: null, | ||
| updatedAt: "2026-02-09T00:00:01.000Z", | ||
| }, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| function makeTerminalActivityEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent { | ||
| return { | ||
| type: "activity", | ||
| threadId: "thread-local-1", | ||
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | ||
| createdAt: "2026-02-09T00:00:02.000Z", | ||
| hasRunningSubprocess: true, | ||
| ...overrides, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix type error causing pipeline failure.
The pipeline fails with TS2719 because Partial<TerminalEvent> allows overriding the type discriminator, causing TypeScript to infer an incompatible union type. The spread of overrides could change type: "started" to any other event type.
🔧 Proposed fix: Use specific override types
-function makeTerminalStartedEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent {
+function makeTerminalStartedEvent(
+ overrides: Partial<Omit<Extract<TerminalEvent, { type: "started" }>, "type">> = {},
+): TerminalEvent {
return {
type: "started",
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
createdAt: "2026-02-09T00:00:01.000Z",
snapshot: {
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
cwd: "/tmp/project",
status: "running",
pid: 1234,
history: "",
exitCode: null,
exitSignal: null,
updatedAt: "2026-02-09T00:00:01.000Z",
},
...overrides,
- };
+ } as TerminalEvent;
}
-function makeTerminalActivityEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent {
+function makeTerminalActivityEvent(
+ overrides: Partial<Omit<Extract<TerminalEvent, { type: "activity" }>, "type">> = {},
+): TerminalEvent {
return {
type: "activity",
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
createdAt: "2026-02-09T00:00:02.000Z",
hasRunningSubprocess: true,
...overrides,
- };
+ } as TerminalEvent;
}Alternatively, a simpler fix using type assertions:
🔧 Simpler fix with type assertions
-function makeTerminalStartedEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent {
- return {
+function makeTerminalStartedEvent(
+ overrides: Partial<TerminalEvent> & { type?: "started" } = {},
+): TerminalEvent {
+ return ({
type: "started",
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
createdAt: "2026-02-09T00:00:01.000Z",
snapshot: {
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
cwd: "/tmp/project",
status: "running",
pid: 1234,
history: "",
exitCode: null,
exitSignal: null,
updatedAt: "2026-02-09T00:00:01.000Z",
},
...overrides,
- };
+ }) as TerminalEvent;
}
-function makeTerminalActivityEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent {
- return {
+function makeTerminalActivityEvent(
+ overrides: Partial<TerminalEvent> & { type?: "activity" } = {},
+): TerminalEvent {
+ return ({
type: "activity",
threadId: "thread-local-1",
terminalId: DEFAULT_THREAD_TERMINAL_ID,
createdAt: "2026-02-09T00:00:02.000Z",
hasRunningSubprocess: true,
...overrides,
- };
+ }) as TerminalEvent;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function makeTerminalStartedEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent { | |
| return { | |
| type: "started", | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| createdAt: "2026-02-09T00:00:01.000Z", | |
| snapshot: { | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| cwd: "/tmp/project", | |
| status: "running", | |
| pid: 1234, | |
| history: "", | |
| exitCode: null, | |
| exitSignal: null, | |
| updatedAt: "2026-02-09T00:00:01.000Z", | |
| }, | |
| ...overrides, | |
| }; | |
| } | |
| function makeTerminalActivityEvent(overrides: Partial<TerminalEvent> = {}): TerminalEvent { | |
| return { | |
| type: "activity", | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| createdAt: "2026-02-09T00:00:02.000Z", | |
| hasRunningSubprocess: true, | |
| ...overrides, | |
| }; | |
| } | |
| function makeTerminalStartedEvent( | |
| overrides: Partial<TerminalEvent> & { type?: "started" } = {}, | |
| ): TerminalEvent { | |
| return ({ | |
| type: "started", | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| createdAt: "2026-02-09T00:00:01.000Z", | |
| snapshot: { | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| cwd: "/tmp/project", | |
| status: "running", | |
| pid: 1234, | |
| history: "", | |
| exitCode: null, | |
| exitSignal: null, | |
| updatedAt: "2026-02-09T00:00:01.000Z", | |
| }, | |
| ...overrides, | |
| }) as TerminalEvent; | |
| } | |
| function makeTerminalActivityEvent( | |
| overrides: Partial<TerminalEvent> & { type?: "activity" } = {}, | |
| ): TerminalEvent { | |
| return ({ | |
| type: "activity", | |
| threadId: "thread-local-1", | |
| terminalId: DEFAULT_THREAD_TERMINAL_ID, | |
| createdAt: "2026-02-09T00:00:02.000Z", | |
| hasRunningSubprocess: true, | |
| ...overrides, | |
| }) as TerminalEvent; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 80-80: TS2719: Type '{ createdAt: string; type: "started"; threadId: string; terminalId: string; snapshot: { status: "running" | "error" | "starting" | "exited"; cwd: string; threadId: string; updatedAt: string; ... 4 more ...; exitSignal: number | null; }; } | ... 5 more ... | { ...; }' is not assignable to type '{ createdAt: string; type: "started"; threadId: string; terminalId: string; snapshot: { status: "running" | "error" | "starting" | "exited"; cwd: string; threadId: string; updatedAt: string; ... 4 more ...; exitSignal: number | null; }; } | ... 5 more ... | { ...; }'. Two different types with this name exist, but they are unrelated. Type '{ createdAt: string; type: "started" | "output"; data?: string; threadId: string; terminalId: string; snapshot: { threadId: string; terminalId: string; cwd: string; status: "running"; pid: number; history: string; exitCode: null; exitSignal: null; updatedAt: string; }; }' is not assignable to type '{ createdAt: string; type: "started"; threadId: string; terminalId: string; snapshot: { status: "running" | "error" | "starting" | "exited"; cwd: string; threadId: string; updatedAt: string; ... 4 more ...; exitSignal: number | null; }; } | ... 5 more ... | { ...; }'.
🤖 Prompt for AI Agents
In `@apps/web/src/store.test.ts` around lines 79 - 109, The type error is caused
because overrides: Partial<TerminalEvent> allows changing the discriminant
"type" and yields an incompatible union; update both makeTerminalStartedEvent
and makeTerminalActivityEvent to accept overrides: Partial<Omit<TerminalEvent,
"type">> (i.e., exclude the "type" field from the override) so the functions
keep their fixed discriminant ("started" or "activity") while still allowing
other fields to be overridden; keep the return type as TerminalEvent and spread
overrides as before.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| }); | ||
| } | ||
|
|
||
| dispose(): void { |
There was a problem hiding this comment.
🟢 Low
src/terminalManager.ts:434 Consider adding a disposed flag that's checked by runWithThreadLock or startSession. Currently, pending open operations can complete after dispose, restarting the subprocess polling timer and leaking resources.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around line 434:
Consider adding a `disposed` flag that's checked by `runWithThreadLock` or `startSession`. Currently, pending `open` operations can complete after `dispose`, restarting the subprocess polling timer and leaking resources.
- add `terminalRunning` to thread state and hydration defaults - apply terminal events in reducer to sync running/open state (close drawer on exit) - wire drawer running-state callbacks and show a Terminal status pill in sidebar - extend store and persistence tests for terminal running/exited behavior
- On open, read legacy transcript files and write them to the new terminal-scoped history path - Remove legacy files after migration and warn if cleanup fails - Update tests to assert migration, content preservation, and legacy file removal
- Attempt `api.terminal.close` when toggling an open terminal off - Fall back to sending `exit` if close is unavailable or fails - Include `api` in callback dependencies to keep behavior current
- Add cross-platform subprocess polling in `TerminalManager` and emit `activity` events when child-process state changes - Update web store to derive `runningTerminalIds` from `activity.hasRunningSubprocess` instead of start/restart events - Extend terminal contracts and tests to include the new `activity` event type Co-authored-by: codex <codex@users.noreply.github.com>
- Pass subprocess test options only when defined in `terminalManager` tests - Narrow web store terminal event fixtures to `started`/`activity` event shapes
juliusmarminge
force-pushed
the
codething/696df84f
branch
from
a303d0f to
b185c14
Compare
1 participant
Summary
terminalRunningto thread state and initialize it across thread creation and hydration pathsAPPLY_TERMINAL_EVENTinstead of handling onlyexitedinlinestarted,restarted,exited, anderroreventsThreadTerminalDrawerrunning-state updates back to the storeTerminalstatus pill in the sidebar when a thread terminal is runningTesting
apps/web/src/store.test.ts: verifiesSET_THREAD_TERMINAL_RUNNING,APPLY_TERMINAL_EVENTstarted handling, and exited handling (stops running + closes drawer)apps/web/src/persistenceSchema.test.ts: verifies persisted thread shape includes terminal state fieldsSummary by CodeRabbit
New Features
Improvements
Tests
Chores