fix (desktop): fix crash on input too big -> pty buffer backpressure#765
fix (desktop): fix crash on input too big -> pty buffer backpressure#765
Conversation
📝 WalkthroughWalkthroughA new InputWriter class is introduced to handle non-blocking writes to PTY by buffering and chunking large writes. The TerminalSession integrates this writer, and TerminalManager dispatches all write operations through it instead of directly to the PTY. Changes
Sequence DiagramsequenceDiagram
participant Manager as TerminalManager
participant InputWriter as InputWriter
participant Queue as Write Queue
participant PTY as PTY Process
Manager->>InputWriter: write(largeData)
alt Data Size < CHUNK_THRESHOLD
InputWriter->>PTY: write(data)
else Data Size >= CHUNK_THRESHOLD
InputWriter->>Queue: enqueue(data)
InputWriter->>InputWriter: processQueue()
end
loop For Each Chunk
InputWriter->>PTY: write(CHUNK_SIZE)
InputWriter->>Queue: dequeue & update
InputWriter->>InputWriter: setImmediate(writeNextChunk)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/terminal/session.ts (1)
202-204: UseinputWriterfor initial commands to match the pattern in manager.ts.
setupDataHandlerbypassesInputWriterwhen writing initial commands directly viasession.pty.write(), whilemanager.tsusessession.inputWriter.write()for all other input to prevent main thread blocking. For consistency and to handle potentially large initial command strings (especially when multiple setup commands are joined with&&), usesession.inputWriter.write(initialCommandString)instead.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/session.ts`:
- Around line 212-216: The test mock for TerminalSession is missing the required
inputWriter, causing flushSession(session: TerminalSession) to throw when it
calls session.inputWriter.dispose(); update the test's TerminalSession mock used
in the failing test to include an inputWriter object with a dispose() stub
(matching other mocks like dataBatcher.dispose and headless.dispose), or if you
prefer the runtime to be defensive, change flushSession to guard the call (e.g.,
use optional chaining when invoking session.inputWriter.dispose()), but the
preferred fix is to add an inputWriter mock so flushSession can call dispose on
a defined object.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal/input-writer.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/input-writer.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/input-writer.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/input-writer.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/input-writer.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal/session.ts (1)
apps/desktop/src/main/lib/terminal/input-writer.ts (1)
InputWriter(28-133)
apps/desktop/src/main/lib/terminal/types.ts (1)
apps/desktop/src/main/lib/terminal/input-writer.ts (1)
InputWriter(28-133)
🪛 GitHub Check: Test
apps/desktop/src/main/lib/terminal/session.ts
[failure] 213-213: TypeError: undefined is not an object (evaluating 'session.inputWriter.dispose')
at flushSession (/home/runner/work/superset/superset/apps/desktop/src/main/lib/terminal/session.ts:213:10)
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/terminal/session.test.ts:109:4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Typecheck
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/main/lib/terminal/input-writer.ts (3)
1-36: LGTM on class structure and constants.The constants are well-documented, and the class setup with proper state management looks correct. The type-only import of
node-ptyis appropriate.
44-62: LGTM on write method logic.The early return for disposed state and the branching between direct and queued writes is well-structured. Error handling with contextual logging follows the coding guidelines.
128-132: LGTM on dispose implementation.The disposal sequence correctly sets the disposed flag first, ensuring any in-flight
setImmediatecallbacks will exit early when they checkisDisposed.apps/desktop/src/main/lib/terminal/types.ts (1)
5-5: LGTM on type definitions.The type-only import and interface extension correctly define the new
inputWriterproperty forTerminalSession.Also applies to: 20-20
apps/desktop/src/main/lib/terminal/manager.ts (2)
167-170: LGTM on write path update.The switch to
inputWriter.write()correctly implements the non-blocking write mechanism. The error handling and session validation remain intact.
391-391: LGTM on prompt refresh update.Using
inputWriter.write("\n")is consistent with the new write path. The single newline character will take the fast direct-write path.apps/desktop/src/main/lib/terminal/session.ts (1)
134-149: LGTM on InputWriter instantiation and session integration.The
InputWriteris correctly instantiated with the PTY process and properly included in the returned session object.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Write one chunk | ||
| const chunk = data.slice(0, CHUNK_SIZE); | ||
| const remaining = data.slice(CHUNK_SIZE); |
There was a problem hiding this comment.
Unicode characters may be split mid-sequence when chunking.
String.slice() operates on UTF-16 code units, not Unicode grapheme clusters. When pasting Unicode text (which this PR specifically aims to support), slicing at arbitrary positions can split surrogate pairs or multi-byte sequences, potentially corrupting characters like emojis or CJK text.
Consider aligning chunk boundaries to avoid splitting characters:
Proposed fix using a boundary-aware slice
- // Write one chunk
- const chunk = data.slice(0, CHUNK_SIZE);
- const remaining = data.slice(CHUNK_SIZE);
+ // Write one chunk, ensuring we don't split surrogate pairs
+ let boundary = Math.min(CHUNK_SIZE, data.length);
+ // If boundary falls between surrogate pair, adjust
+ if (boundary < data.length) {
+ const charCode = data.charCodeAt(boundary - 1);
+ // High surrogate: 0xD800-0xDBFF
+ if (charCode >= 0xD800 && charCode <= 0xDBFF) {
+ boundary--; // Don't split the pair
+ }
+ }
+ const chunk = data.slice(0, boundary);
+ const remaining = data.slice(boundary);📝 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.
| // Write one chunk | |
| const chunk = data.slice(0, CHUNK_SIZE); | |
| const remaining = data.slice(CHUNK_SIZE); | |
| // Write one chunk, ensuring we don't split surrogate pairs | |
| let boundary = Math.min(CHUNK_SIZE, data.length); | |
| // If boundary falls between surrogate pair, adjust | |
| if (boundary < data.length) { | |
| const charCode = data.charCodeAt(boundary - 1); | |
| // High surrogate: 0xD800-0xDBFF | |
| if (charCode >= 0xD800 && charCode <= 0xDBFF) { | |
| boundary--; // Don't split the pair | |
| } | |
| } | |
| const chunk = data.slice(0, boundary); | |
| const remaining = data.slice(boundary); |
| export function flushSession(session: TerminalSession): void { | ||
| session.inputWriter.dispose(); | ||
| session.dataBatcher.dispose(); | ||
| session.headless.dispose(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the session.ts file and examine the relevant code
find . -name "session.ts" -path "*apps/desktop*" | head -5Repository: superset-sh/superset
Length of output: 111
🏁 Script executed:
# Examine the session.ts file to understand the flushSession function and TerminalSession type
cat -n ./apps/desktop/src/main/lib/terminal/session.ts | sed -n '1,250p'Repository: superset-sh/superset
Length of output: 6719
🏁 Script executed:
# Find and examine the types.ts file for TerminalSession type definition
fd "types.ts" -path "*apps/desktop/src/main/lib/terminal*" --type fRepository: superset-sh/superset
Length of output: 232
🏁 Script executed:
# Find and examine the types.ts file for TerminalSession type definition
fd "types.ts" -path "*apps/desktop/src/main/lib/terminal*" -type fRepository: superset-sh/superset
Length of output: 232
🏁 Script executed:
# Find and examine the types.ts file for TerminalSession type definition
find ./apps/desktop/src/main/lib/terminal -name "types.ts"Repository: superset-sh/superset
Length of output: 109
🏁 Script executed:
# Examine the types.ts file to understand TerminalSession interface
cat -n ./apps/desktop/src/main/lib/terminal/types.tsRepository: superset-sh/superset
Length of output: 1849
🏁 Script executed:
# Find test files related to session
find ./apps/desktop -name "*.test.ts" -o -name "*.test.tsx" | xargs grep -l "flushSession\|session" 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 169
🏁 Script executed:
# Examine the session.test.ts file
cat -n ./apps/desktop/src/main/lib/terminal/session.test.tsRepository: superset-sh/superset
Length of output: 3677
🏁 Script executed:
# Check if there's a git history or any recent changes to understand if this is a new issue
git log --oneline -20 ./apps/desktop/src/main/lib/terminal/session.tsRepository: superset-sh/superset
Length of output: 98
🏁 Script executed:
# Check the git diff to see what changes were made in that commit
git show 182a539 --statRepository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
# Check the diff specifically for session.ts
git show 182a539 -- apps/desktop/src/main/lib/terminal/session.tsRepository: superset-sh/superset
Length of output: 5816
🏁 Script executed:
# Check if inputWriter is ever made optional or if there are other callers of flushSession
rg "flushSession" apps/desktop/src/main/lib/terminal/Repository: superset-sh/superset
Length of output: 537
🏁 Script executed:
# Check how flushSession is used in manager.ts
rg -A 3 -B 3 "flushSession\(session" apps/desktop/src/main/lib/terminal/manager.tsRepository: superset-sh/superset
Length of output: 333
🏁 Script executed:
# Check if there are any other test files for terminal/session functionality
find apps/desktop -name "*session*test*" -o -name "*test*session*" | grep -i terminalRepository: superset-sh/superset
Length of output: 114
🏁 Script executed:
# Check manager.test.ts for flushSession usage
rg -A 10 -B 5 "flushSession" apps/desktop/src/main/lib/terminal/manager.test.ts | head -50Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Let's look at the full test to understand what it's actually testing
sed -n '87,115p' apps/desktop/src/main/lib/terminal/session.test.tsRepository: superset-sh/superset
Length of output: 662
The test mock is incomplete and will cause flushSession to fail.
The test creates a partial mock of TerminalSession that omits the inputWriter property (line 104-107), but flushSession unconditionally calls session.inputWriter.dispose() on line 213. This will throw "Cannot read property 'dispose' of undefined" at runtime.
Fix the mock to include inputWriter:
Add inputWriter to test mock
const mockSession = {
dataBatcher: mockDataBatcher,
headless: mockHeadless,
+ inputWriter: { dispose: () => { /* track if called */ } },
} as unknown as TerminalSession;Alternatively, use optional chaining in flushSession to be defensive, though the type system defines inputWriter as required.
📝 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.
| export function flushSession(session: TerminalSession): void { | |
| session.inputWriter.dispose(); | |
| session.dataBatcher.dispose(); | |
| session.headless.dispose(); | |
| } | |
| export function flushSession(session: TerminalSession): void { | |
| session.inputWriter?.dispose(); | |
| session.dataBatcher?.dispose(); | |
| session.headless?.dispose(); | |
| } |
🧰 Tools
🪛 GitHub Check: Test
[failure] 213-213: TypeError: undefined is not an object (evaluating 'session.inputWriter.dispose')
at flushSession (/home/runner/work/superset/superset/apps/desktop/src/main/lib/terminal/session.ts:213:10)
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/terminal/session.test.ts:109:4)
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/session.ts` around lines 212 - 216, The
test mock for TerminalSession is missing the required inputWriter, causing
flushSession(session: TerminalSession) to throw when it calls
session.inputWriter.dispose(); update the test's TerminalSession mock used in
the failing test to include an inputWriter object with a dispose() stub
(matching other mocks like dataBatcher.dispose and headless.dispose), or if you
prefer the runtime to be defensive, change flushSession to guard the call (e.g.,
use optional chaining when invoking session.inputWriter.dispose()), but the
preferred fix is to add an inputWriter mock so flushSession can call dispose on
a defined object.
|
fixed in persistence pr |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.