Feat/shell integrated resolved#41
Merged
Merged
Conversation
…ll creack/pty v1.1.24
…stubs, shell detection
- emitOutput and monitorExit now check wailsApp != nil before emitting Wails events, preventing nil pointer dereference in unit tests. - Fix TestTerminalExit race condition: check local cmd.ProcessState instead of s.cmd (which gets replaced by auto-restart). - All 16 tests pass across 2 packages.
… addon-webgl, addon-web-links)
…der, collapse, split layout)
…acking - Remove OutputPane import from App.tsx, add TerminalComponent import - Remove outputPaneOpen state, tabOutputRef, outputPaneOpenRef - Simplify tabPaneStateRef type to only track historyOpen - Clean tabOutputRef reads/writes from finalizeCloseTab, openNewCommandTab, openTab, handleSelectTab, flushStreamBuffer, runCommandDirect, handleSelectRecord, and confirmClearHistory - Remove toggleOutput shortcut handler from App.tsx and SHORTCUTS registry from shortcuts.ts - Remove OutputPane JSX from App.tsx layout - Remove toggleOutput shortcut label from OutputPane.tsx
…d Terminal component
…mand cmd-output events to xterm.js terminal
RunCommand now emits pty-output events instead of cmd-output, matching the xterm.js terminal subscription in Terminal.tsx. This fixes the UAT gap where command output was invisible after OutputPane removal in 17-02.
Removes streamLines state, streamBufferRef, streamFlushRef, streamLinesRef, flushStreamBuffer, cmd-output event subscription, MAX_STREAM_LINES constant, and all setStreamLines() calls. RunCommand now emits pty-output events which render directly in the xterm.js terminal.
- Test PTY Write with working dir (cd sandwich) - Test PTY Write without working dir (plain script) - Test error when db.GetCommand fails - Test error when terminalSvc.Write fails - Test terminalSvc assignment in ServiceStartup - Test shellQuoteDir escaping - Test no history persistence from Run path - Add terminalSvc package-level var to app.go
- Rewrite RunCommand to write cd-sandwich-wrapped command to TerminalService.Write instead of executor.ExecuteScript subprocess - Add hasExplicitWorkingDir helper to only cd-sandwich when a working dir is explicitly set (per-command or global default) - Assign terminalSvc = s in TerminalService.ServiceStartup - Remove db.AddExecution call — fire-and-forget per D-01 - All 10 new execution_service tests pass
- Remove setSelectedRecord/setHistoryPaneOpen side-effects from Run path - Remove loadHistory call (no history persisted from PTY write) - Remove exitCode logic (unknown in fire-and-forget PTY execution) - Remove errRecord construction in catch block - Keep executing state tracking and active-tab toast gating - Preserve selectedRecord/historyPaneOpen state for HistoryPane - Dependency array simplified to [t]
…/Copy Out toolbar buttons
Task 2: Copy Cmd reads lastCommandRef from cmd-executing event and writes to clipboard. Copy Out uses TerminalHandle.getSelection() via xterm.js. Buttons styled to match existing terminal toolbar, only visible when terminal is expanded.
… button in toolbar - Remove Copy Cmd/Copy Out overlay buttons from terminal pane - Add Copy button next to Clear in terminal divider that copies accumulated pty-output - pty-output listener in App.tsx accumulates output into lastOutputRef - cmd-executing event resets output buffer for each new command - Fix main content scrollability (overflow-y: auto on .center-area-editor and .main-content)
…ollbar size - isOutputBuffering ref gates pty-output accumulation (only after cmd-executing) - Terminal scrollbar now matches main section: width 4px, scrollbar-thumb color, 4px radius
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a PTY-backed TerminalService and platform PTY helpers, implements a frontend xterm TerminalComponent, refactors execution to emit final command strings (CmdExecuting), updates event/type contracts for PTY events, rewires App to a terminal split pane, and includes tests and agent skill/guideline docs. ChangesPTY Terminal Implementation
Frontend Terminal Component and UI Integration
Agent Skills and Configuration
Sequence Diagram (high-level runtime flow): sequenceDiagram
participant User
participant App
participant ExecutionService
participant TerminalService
participant FrontendTerminal
User->>App: trigger run (onExecute)
App->>ExecutionService: RunCommand (builds FinalCmd, emits cmdExecuting)
ExecutionService->>TerminalService: (event consumed) cmdExecuting/Start/Write
TerminalService->>FrontendTerminal: emit ptyOutput / ptyExit / ptyCleared
FrontendTerminal->>User: render terminal output / handle exit
Estimated code review effort:
🚥 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
.agents/skills/todo-scanner/SKILL.md (1)
78-83: ⚡ Quick winConsider simplifying the collision handling flow.
The current approach uses
rmfollowed by conditionalgit rm --quiet --ignore-unmatch. While functionally correct, the flow could be more direct:**If `<name>.resolved.md` already exists (collision):** Do NOT use `git mv` — it will fail. Instead: 1. Append the full content of `todos/<name>.pending.md` (including its resolution header) to the existing `todos/<name>.resolved.md` 2. Check if inside a git repo (`git rev-parse --is-inside-work-tree 2>/dev/null`) - If true: `git rm todos/<name>.pending.md` to remove and stage deletion - If false: `rm todos/<name>.pending.md` to delete the file 3. Stage the updated resolved file: `git add todos/<name>.resolved.md` (only if in a git repo)This makes the intent clearer and avoids the
--ignore-unmatchflag by choosing the right deletion command upfront.🤖 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 @.agents/skills/todo-scanner/SKILL.md around lines 78 - 83, Update the collision-handling steps for `<name>.pending.md` -> `<name>.resolved.md`: instead of `rm` then conditional `git rm --quiet --ignore-unmatch`, first append the pending file (including resolution header) to the existing `todos/<name>.resolved.md`, then detect repo membership with `git rev-parse --is-inside-work-tree` and if true run `git rm todos/<name>.pending.md` to remove and stage the deletion, otherwise run `rm todos/<name>.pending.md`; finally, if inside a git repo run `git add todos/<name>.resolved.md` to stage the updated resolved file (skip staging when not in a repo).
🤖 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.
Inline comments:
In @.agents/skills/wails-feature/SKILL.md:
- Around line 20-25: Update the Regenerate TypeScript Bindings step in SKILL.md
to match the repository workflow: replace the incorrect instruction referencing
`wails generate module` with the correct command used by the Makefile (`wails3
generate bindings`) or call the Makefile target (`make generate`); update the
explanatory text that follows (e.g., the sentence mentioning
`frontend/wailsjs/go/main/App.ts` and `App.d.ts`) so it still describes the
files that are updated after running `wails3 generate bindings`/`make generate`.
In `@execution_service_test.go`:
- Around line 49-64: The working_dir test fixtures hardcode a "darwin" key so on
non-macOS systems GetCurrentOS() doesn't pick up the working dir; update the
fixtures in TestRunCommand_FinalCmdWithWorkingDir and
TestRunCommand_FinalCmdMultilineScript (the calls that use testDBCreateCommand)
to set the OS key using runtime.GOOS instead of "darwin" so RunCommand will read
the correct working dir for the current platform and the FinalCmd assertions
(record.FinalCmd) succeed.
- Around line 1-7: This new test file violates the repo rule that all tests live
in db_test.go—remove execution_service_test.go and move its test cases into
db_test.go; paste the test functions (keeping their original names) into
db_test.go or merge their logic into the existing core tests
(TestFreshDBMigrations, TestExistingDBIdempotent, TestRollbackTo) as
appropriate, ensure imports (e.g., github.com/wailsapp/wails/v3/pkg/application)
are consolidated at the top of db_test.go, avoid duplicate package declarations,
and run go test ./... to verify no conflicts or duplicate test names remain.
In `@execution_service.go`:
- Around line 124-132: The emitted PTY command is using the raw resolvedScript
(from ReplaceTemplateVars) which bypasses stripShebang used by
Executor.OpenInTerminal; apply stripShebang to the resolvedScript before
trimming/adding the trailing newline so both branches (when
s.hasExplicitWorkingDir is true and false) use the shebang-stripped content;
update references around resolvedScript, stripShebang, cmdLine,
s.resolveWorkingDir and s.hasExplicitWorkingDir to build cmdLine from the
stripped script (ensuring you preserve the existing TrimRight + "\n" behavior
and the cd prefix via shellQuoteDir).
In `@frontend/src/App.tsx`:
- Around line 860-879: runCommandDirect currently ignores ExecutionRecord
results from the RunCommand call; change the try block to capture the returned
record (e.g. const result = await RunCommand(commandId, variables)) and then
explicitly treat result.Error or result.ExitCode === -1 as a failure: call
toast.error(...) (using t('toast.commandFailed', { code: result?.ExitCode ?? -1
})) and keep the terminal expanded / spinner state appropriate before letting
finally run; reference the runCommandDirect function and the RunCommand call and
check the ExecutionRecord fields Error and ExitCode to decide failure handling.
In `@frontend/src/components/Terminal.tsx`:
- Around line 41-45: The imperative handlers (useImperativeHandle) call Clear,
Write, Resize, and Start and currently only log errors to console; update each
async Wails call to use .then/.catch (or try/catch) and call the sonner toast
function (e.g., toast.error or toast) to surface failures to the user, and set a
component state flag (e.g., backendAvailable or terminalDisabled) when those
calls fail so UI controls and terminalRef.current?.clear()/write/etc. become
disabled/readonly; modify the handlers referenced by useImperativeHandle and any
related methods that call Clear/Write/Resize/Start so they show a toast on error
and flip the disable flag to prevent interactive controls when the backend is
unavailable.
- Around line 236-246: Remove the raw PTY logging calls to avoid leaking
terminal output and exit info: delete the console.log({output}) inside the
Events.On handler for eventNames.ptyOutput (where cleanupOutput is defined) and
delete or guard the console.log in the Events.On handler for eventNames.ptyExit
(cleanupExit). If you need retained logs for local debugging, wrap both
console.log calls behind an explicit dev-only flag (e.g., process.env.NODE_ENV
=== 'development' or an isDevConstant) so production builds never log PTY output
or exit details.
In `@terminal_service_test.go`:
- Line 1: The new test file terminal_service_test.go violates the repo's test
placement rule; either move its tests into the canonical db_test.go (merge the
test functions and any helpers into db_test.go, keeping test names like Test...
and any setup/teardown helpers such as TestMain or helper functions) or, if you
intend to change the convention, update the repository guideline in the same PR
to reflect the new placement; ensure there are no duplicate test definitions and
that running go test ./... still executes the moved tests.
- Around line 149-150: The test builds a single string for the shell argument
which causes exec.Command in ptyStart to receive one token (e.g., shellFlag + `
-c "..."`) instead of separate argv entries; update the test to pass args as
separate tokens (e.g., call ptyStart with shellPath and separate args like "-c",
"sleep 60" or change the test to pass a []string/variadic args so
exec.Command(shellPath, "-c", "sleep 60") is invoked) and adjust both
occurrences around the ptyStart calls (the lines using ptyStart in
terminal_service_test.go). Also follow the repo guideline by moving the tests
out of terminal_service_test.go into db_test.go (remove the duplicated tests
from terminal_service_test.go and add them into db_test.go), ensuring the calls
reference the existing ptyStart helper and any test helpers remain available.
In `@terminal_service.go`:
- Around line 117-125: The enqueueOutput function currently drops PTY output
silently when s.outputCh is full (select default); modify it to avoid silent
data loss by either blocking with a bounded timeout or implementing a
non-destructive backpressure strategy: locate enqueueOutput and s.outputCh, and
replace the select-default with logic that attempts to send to ch and on full
either (a) wait up to a short timeout then log a warning including dropped chunk
metadata and/or increment a dropped counter, or (b) rotate the buffer by
removing the oldest entry and enqueueing the new one, ensuring you emit a log
via the service logger (or expose a dropped counter) so downstream code can
observe lost output instead of silently dropping it.
- Around line 354-363: In TerminalService.Resize, validate and sanitize the
incoming cols/rows before persisting to lastSize or calling ptyResize: ensure
cols and rows are within 1..65535 (or choose a safe min/max), reject or clamp
out-of-range/zero/negative values and return an error for invalid input; only
after validation assign s.lastSize = ptyWinsize{Rows: uint16(rows), Cols:
uint16(cols)} and then call ptyResize(s.ptmx, cols, rows). This change should be
applied inside the Resize method (referencing TerminalService.Resize,
s.lastSize, ptyWinsize and ptyResize) and keep the existing mutex usage.
- Around line 219-223: ServiceStartup currently calls s.Start(80, 24)
synchronously which can hard-fail on platforms where PTY backend is unavailable
(e.g., ptyStart returns "not yet implemented"); change ServiceStartup to not
hard-fail the whole app: set terminalSvc = s and call s.startEmitter(), then
call s.Start(80, 24) but catch the specific platform/PTY error (or detect the
"not yet implemented" message) and log/warn instead of returning the error so
startup continues, or alternatively defer calling s.Start until the first
terminal is actually requested; reference ServiceStartup, s.Start, startEmitter,
terminalSvc and ptyStart when making the change.
In `@terminal_unix.go`:
- Around line 15-22: ptyStart currently starts the child process without setting
a dedicated process group, so killProcessGroup (which uses
syscall.Kill(-pid,...)) may not target the child tree; update ptyStart to set
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} (or ensure Setpgid is true
if SysProcAttr exists) before calling pty.StartWithSize so the spawned process
has its own pgid and negative-PID kills from killProcessGroup reliably affect
the child process group.
---
Nitpick comments:
In @.agents/skills/todo-scanner/SKILL.md:
- Around line 78-83: Update the collision-handling steps for `<name>.pending.md`
-> `<name>.resolved.md`: instead of `rm` then conditional `git rm --quiet
--ignore-unmatch`, first append the pending file (including resolution header)
to the existing `todos/<name>.resolved.md`, then detect repo membership with
`git rev-parse --is-inside-work-tree` and if true run `git rm
todos/<name>.pending.md` to remove and stage the deletion, otherwise run `rm
todos/<name>.pending.md`; finally, if inside a git repo run `git add
todos/<name>.resolved.md` to stage the updated resolved file (skip staging when
not in a repo).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4b30475-c9d9-4eff-84c2-9303de867a2e
⛔ Files ignored due to path filters (4)
.planning/ROADMAP.mdis excluded by!.planning/*.planning/STATE.mdis excluded by!.planning/*frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.agents/skills/add-shadcn-component/SKILL.md.agents/skills/todo-scanner/SKILL.md.agents/skills/wails-feature/SKILL.md.opencode/prompts/executor-guideline.md.opencode/prompts/explore-guideline.mdapp.goevent_service.goexecution_service.goexecution_service_test.goexecutor.gofrontend/package.jsonfrontend/src/App.tsxfrontend/src/components/CommandDetail.tsxfrontend/src/components/CommandDetailTab.tsxfrontend/src/components/HistoryPane.tsxfrontend/src/components/KeyboardShortcutsDialog.tsxfrontend/src/components/OutputPane.tsxfrontend/src/components/Terminal.tsxfrontend/src/lib/shortcuts.tsfrontend/src/locales/en.jsonfrontend/src/style.cssfrontend/src/types.tsfrontend/src/wails/events.tsgo.modmain.gomodels.goopencode.jsonterminal_service.goterminal_service_test.goterminal_unix.goterminal_windows.go
💤 Files with no reviewable changes (4)
- frontend/src/components/HistoryPane.tsx
- frontend/src/components/CommandDetailTab.tsx
- frontend/src/types.ts
- frontend/src/components/CommandDetail.tsx
Comment on lines
+1
to
+7
| package main | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/wailsapp/wails/v3/pkg/application" | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move these cases into db_test.go.
This new file conflicts with the repository’s Go test layout rule. Please fold these tests into db_test.go instead of adding another *_test.go file.
As per coding guidelines, *_test.go: Go tests are in db_test.go only; run with go test ./...; three core migration tests exist: TestFreshDBMigrations, TestExistingDBIdempotent, TestRollbackTo.
🤖 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 `@execution_service_test.go` around lines 1 - 7, This new test file violates
the repo rule that all tests live in db_test.go—remove execution_service_test.go
and move its test cases into db_test.go; paste the test functions (keeping their
original names) into db_test.go or merge their logic into the existing core
tests (TestFreshDBMigrations, TestExistingDBIdempotent, TestRollbackTo) as
appropriate, ensure imports (e.g., github.com/wailsapp/wails/v3/pkg/application)
are consolidated at the top of db_test.go, avoid duplicate package declarations,
and run go test ./... to verify no conflicts or duplicate test names remain.
Comment on lines
+149
to
+150
| ptmx, c, err := ptyStart(shellPath, shellFlag+" -c \"sleep 60\"", 80, 24) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify single-argument shell construction patterns that rely on implicit parsing.
rg -n -C2 'shellFlag\s*\+\s*".*-c' --type=go
rg -n -C2 'exec\.Command\(' --type=goRepository: loco1842/cmdex
Length of output: 4256
Fix PTY shell argv construction in terminal_service_test.go (lines 149-150, also 194-195) and move tests per guideline
terminal_service_test.gobuildsshellFlagby appending-c "..."into a single string argument, but on UnixptyStartusesexec.Command(shellPath, shellFlag)—so-cisn’t passed as a separate argv token and the shell can mis-handle it.- The repo guideline requires Go tests to be centralized in
db_test.goonly;terminal_service_test.goadds test coverage outside that file.
🤖 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 `@terminal_service_test.go` around lines 149 - 150, The test builds a single
string for the shell argument which causes exec.Command in ptyStart to receive
one token (e.g., shellFlag + ` -c "..."`) instead of separate argv entries;
update the test to pass args as separate tokens (e.g., call ptyStart with
shellPath and separate args like "-c", "sleep 60" or change the test to pass a
[]string/variadic args so exec.Command(shellPath, "-c", "sleep 60") is invoked)
and adjust both occurrences around the ptyStart calls (the lines using ptyStart
in terminal_service_test.go). Also follow the repo guideline by moving the tests
out of terminal_service_test.go into db_test.go (remove the duplicated tests
from terminal_service_test.go and add them into db_test.go), ensuring the calls
reference the existing ptyStart helper and any test helpers remain available.
Comment on lines
+15
to
+22
| func ptyStart(shellPath, shellFlag string, rows, cols int) (*os.File, *exec.Cmd, error) { | ||
| cmd := exec.Command(shellPath, shellFlag) | ||
| cmd.Env = os.Environ() | ||
|
|
||
| ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: uint16(rows), Cols: uint16(cols)}) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("pty.StartWithSize: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that process-group setup is absent/present where negative-pgid kill is used.
rg -n -C3 'syscall\.Kill\(-pid' --type=go
rg -n -C3 'SysProcAttr|Setpgid' --type=goRepository: loco1842/cmdex
Length of output: 581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="terminal_unix.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE at repo root"
exit 1
fi
echo "== terminal_unix.go: relevant sections =="
nl -ba "$FILE" | sed -n '1,120p'
echo
echo "== Search for Setpgid/SysProcAttr usage in Go files =="
rg -n --hidden --no-ignore -S 'SysProcAttr|Setpgid' --type=goRepository: loco1842/cmdex
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="terminal_unix.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE at repo root"
ls -la
exit 1
fi
echo "== terminal_unix.go: first 140 lines (with line numbers) =="
cat -n "$FILE" | sed -n '1,140p'
echo
echo "== Search for SysProcAttr/Setpgid usage in Go files =="
rg -n --hidden --no-ignore -S 'SysProcAttr|Setpgid' --type=goRepository: loco1842/cmdex
Length of output: 1533
Set a dedicated process group in ptyStart so killProcessGroup’s negative-PID kills reliably target the child tree.
killProcessGroup calls syscall.Kill(-pid, ...), which sends signals to the process group ID equal to pid. ptyStart currently never sets cmd.SysProcAttr.Setpgid, so the child may not belong to that process group, making shutdown unreliable.
Proposed fix
func ptyStart(shellPath, shellFlag string, rows, cols int) (*os.File, *exec.Cmd, error) {
cmd := exec.Command(shellPath, shellFlag)
cmd.Env = os.Environ()
+ cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: uint16(rows), Cols: uint16(cols)})
if err != nil {
return nil, nil, fmt.Errorf("pty.StartWithSize: %w", err)
}
return ptmx, cmd, nil
}📝 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.
Suggested change
| func ptyStart(shellPath, shellFlag string, rows, cols int) (*os.File, *exec.Cmd, error) { | |
| cmd := exec.Command(shellPath, shellFlag) | |
| cmd.Env = os.Environ() | |
| ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: uint16(rows), Cols: uint16(cols)}) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("pty.StartWithSize: %w", err) | |
| } | |
| func ptyStart(shellPath, shellFlag string, rows, cols int) (*os.File, *exec.Cmd, error) { | |
| cmd := exec.Command(shellPath, shellFlag) | |
| cmd.Env = os.Environ() | |
| cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | |
| ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: uint16(rows), Cols: uint16(cols)}) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("pty.StartWithSize: %w", err) | |
| } |
🤖 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 `@terminal_unix.go` around lines 15 - 22, ptyStart currently starts the child
process without setting a dedicated process group, so killProcessGroup (which
uses syscall.Kill(-pid,...)) may not target the child tree; update ptyStart to
set cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} (or ensure Setpgid is
true if SysProcAttr exists) before calling pty.StartWithSize so the spawned
process has its own pgid and negative-PID kills from killProcessGroup reliably
affect the child process group.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/Terminal.tsx (1)
44-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop sending PTY calls after the first backend failure.
After Line 229 or Line 236 rejects, the terminal stays interactive, so every keystroke, resize, and
cmdExecutingevent can generate another rejected promise and toast. Flip abackendAvailableflag/ref on the first failure and short-circuit these handlers until the session is restarted.Also applies to: 229-239, 276-279
🤖 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 `@frontend/src/components/Terminal.tsx` around lines 44 - 47, Add a backendAvailable boolean/ref (initialized true) and in the Clear() catch handler (and any other PTY promise catch sites that currently call toast.error) set backendAvailable = false on the first rejection and include the error in the log; then short‑circuit all PTY interaction handlers (e.g., onData/onKey/onResize/handleCmdExecuting or wherever keystrokes, resizes, and cmdExecuting send PTY calls) to return immediately when backendAvailable is false so they don't issue further promises or toasts; ensure backendAvailable is reset to true when the session is explicitly restarted or reconnected.
🤖 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.
Inline comments:
In `@frontend/src/components/Terminal.tsx`:
- Around line 216-219: The frontend starts the PTY at a fixed 80x24 because
Resize(term.cols, term.rows) is called before Start() finishes, causing false
resize errors and a wrong initial shell size; change the startup flow so the PTY
is booted with the fitted terminal size instead of racing a Resize call: either
modify Start (or the Start invocation) to accept initialCols and initialRows
(pass term.cols and term.rows) and have the backend use those when booting, or
await Start completing successfully before calling Resize and suppress the
resize toast while Start is pending; update references to Resize(term.cols,
term.rows), Start(), and the resize error handling (toast.error) accordingly so
the initial size is applied atomically and no spurious error toast is shown.
---
Duplicate comments:
In `@frontend/src/components/Terminal.tsx`:
- Around line 44-47: Add a backendAvailable boolean/ref (initialized true) and
in the Clear() catch handler (and any other PTY promise catch sites that
currently call toast.error) set backendAvailable = false on the first rejection
and include the error in the log; then short‑circuit all PTY interaction
handlers (e.g., onData/onKey/onResize/handleCmdExecuting or wherever keystrokes,
resizes, and cmdExecuting send PTY calls) to return immediately when
backendAvailable is false so they don't issue further promises or toasts; ensure
backendAvailable is reset to true when the session is explicitly restarted or
reconnected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d44fac6-8863-4a08-97ca-12269d0d1df3
📒 Files selected for processing (10)
.agents/skills/todo-scanner/SKILL.md.agents/skills/wails-feature/SKILL.mdexecution_service.goexecution_service_test.gofrontend/src/App.tsxfrontend/src/components/Terminal.tsxterminal_service.goterminal_service_test.goterminal_unix.goterminal_windows.go
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/wails-feature/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (7)
- terminal_unix.go
- execution_service.go
- .agents/skills/todo-scanner/SKILL.md
- execution_service_test.go
- terminal_service_test.go
- frontend/src/App.tsx
- terminal_service.go
Comment on lines
+216
to
+219
| Resize(term.cols, term.rows).catch((err) => { | ||
| console.error('resize failed:', err); | ||
| toast.error('Terminal resize failed'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Start the PTY with the fitted size.
Line 216 sends Resize(term.cols, term.rows) before Line 283's Start() has resolved, and Start still boots the backend at 80x24. On a normal slow startup that can raise a false resize toast and leave the shell at the wrong size until another resize happens.
Suggested fix
if (containerRef.current) {
term.open(containerRef.current);
requestAnimationFrame(() => {
fitAddon.fit();
- Resize(term.cols, term.rows).catch((err) => {
- console.error('resize failed:', err);
- toast.error('Terminal resize failed');
- });
+ void Start(term.cols, term.rows).catch((err) => {
+ console.error('terminal start failed:', err);
+ toast.error('Terminal start failed');
+ });
if (!skipTransition) {
containerRef.current!.style.opacity = '1';
}
});
}
@@
- Start(80, 24).catch((err) => {
- console.error('terminal start failed:', err);
- toast.error('Terminal start failed');
- });Also applies to: 283-286
🤖 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 `@frontend/src/components/Terminal.tsx` around lines 216 - 219, The frontend
starts the PTY at a fixed 80x24 because Resize(term.cols, term.rows) is called
before Start() finishes, causing false resize errors and a wrong initial shell
size; change the startup flow so the PTY is booted with the fitted terminal size
instead of racing a Resize call: either modify Start (or the Start invocation)
to accept initialCols and initialRows (pass term.cols and term.rows) and have
the backend use those when booting, or await Start completing successfully
before calling Resize and suppress the resize toast while Start is pending;
update references to Resize(term.cols, term.rows), Start(), and the resize error
handling (toast.error) accordingly so the initial size is applied atomically and
no spurious error toast is shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI/UX Changes
Documentation