terminal#12
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end terminal support: OS shell detection and PTY sessions in the Rust backend (portable-pty), Tauri invoke handlers and JS wrappers, xterm.js-based TerminalTab component, TerminalPanel UI for shell selection/rename/kill, and app layout integration. ChangesTerminal/PTY Feature
Sequence DiagramsequenceDiagram
participant User
participant TerminalPanel as TerminalPanel UI
participant JS as JS Bridge (src/lib/commands/terminal.js)
participant Rust as Rust Backend (src-tauri)
participant PTY as PTY/Shell
participant TerminalTab as TerminalTab UI
User->>TerminalPanel: Click "New Terminal"
TerminalPanel->>JS: terminalListShells()
JS->>Rust: invoke terminal_list_shells
Rust->>JS: return ShellInfo[]
JS->>TerminalPanel: shells array
TerminalPanel->>TerminalPanel: render shell picker
User->>TerminalPanel: select shell
TerminalPanel->>JS: terminalCreate(sessionId, cwd, cols, rows, shell, shellArgs)
JS->>Rust: invoke terminal_create
Rust->>PTY: open PTY and spawn shell
Rust->>Rust: start reader thread
Rust->>JS: emit terminal:data:{sid} (base64)
JS->>TerminalTab: deliver event
TerminalTab->>TerminalTab: render output in xterm
User->>TerminalTab: type command
TerminalTab->>JS: terminalWrite(sessionId, data)
JS->>Rust: invoke terminal_write
Rust->>PTY: write to PTY writer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 5
🧹 Nitpick comments (1)
src/lib/components/panels/TerminalPanel.svelte (1)
56-63: ⚡ Quick winExpose dropdown state to assistive tech.
The shell-picker toggle should include
aria-expandedandaria-controlsso screen readers can track open/closed state.♿ Suggested patch
<button type="button" aria-label="Pick shell" + aria-expanded={dropdownOpen} + aria-controls="terminal-shell-picker" onclick={() => (dropdownOpen = !dropdownOpen)} class="flex items-center justify-center px-1.5 rounded-r-md border border-dashed border-border/60 text-muted-foreground hover:text-foreground hover:border-border hover:bg-muted/50 transition-colors {dropdownOpen ? 'bg-muted/50 text-foreground border-border' : ''}" > <ChevronDown size={12} class="transition-transform {dropdownOpen ? 'rotate-180' : ''}" /> </button> @@ - <div class="absolute left-2 right-2 top-full mt-1 z-50 rounded-md border bg-popover shadow-md overflow-hidden"> + <div id="terminal-shell-picker" class="absolute left-2 right-2 top-full mt-1 z-50 rounded-md border bg-popover shadow-md overflow-hidden">Also applies to: 72-73
🤖 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 `@src/lib/components/panels/TerminalPanel.svelte` around lines 56 - 63, The shell-picker toggle button that flips the local dropdownOpen state should expose that state to assistive tech by adding aria-expanded={dropdownOpen} and aria-controls="shell-dropdown" (or another unique id) to the button, and ensure the dropdown element rendered when dropdownOpen is used has id="shell-dropdown" (and appropriate role like "menu" or "listbox") so screen readers can associate the control with the popup; apply the same aria-expanded/aria-controls change to the other toggle referenced (the second dropdown toggle) and ensure ids are unique if both dropdowns exist.
🤖 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 @.claude/settings.json:
- Line 11: Replace the Bash(...) wrapper around the PowerShell cmdlet with a
PowerShell(...) wrapper and remove the hardcoded Windows path: change the
"Bash(Get-ChildItem -Path \"d:\\\\takerest\\\\gits\\\\takerest\" -Force)" string
to use PowerShell(Get-ChildItem -Path $Env:TAKEREST_ROOT -Force) or
PowerShell(Get-ChildItem -Path (Join-Path $Env:USERPROFILE
'takerest\\gits\\takerest') -Force) so the command runs in PowerShell and the
path is environment-driven/portable; ensure proper quoting/escaping for the JSON
string and reference the original Bash(...), Get-ChildItem, and the settings
entry when making the edit.
In `@src-tauri/src/commands/terminal.rs`:
- Around line 278-286: The current code silently does nothing when a session id
is missing (the if let Some(session) = sessions.get_mut(&session_id) { ... }
branches), which hides state drift; update these code paths (where you access
sessions, e.g., the write path using session.writer.write_all(data.as_bytes()),
the resize path, and the close/send paths that use sessions.get/_get_mut) to
return an explicit error when the session is not found instead of succeeding.
Replace the if-let-no-op with a match or an early-return check that returns
Err(...) (use a clear message including the session_id, e.g., format!("session
not found: {}", session_id) converted to the function's error type) so callers
receive a failure when the session id is unknown.
- Around line 259-267: The current code uses
state.sessions.lock().unwrap().insert(session_id, PtySession { ... }) which will
silently replace an existing session and leak the previous PTY; change this to
first check for an existing entry for session_id and avoid replacing it (return
an error) or, if replacement is intended, first remove the existing session and
gracefully terminate it before inserting the new one. Concretely, use
state.sessions.lock().unwrap().entry(session_id) to detect Existing vs Vacant
and either return Err/early when the entry is Occupied, or call remove/take the
old PtySession and call its child.kill()/cleanup methods (and close its writer)
before inserting the new PtySession so you don’t leave the previous PTY running.
In `@src/lib/components/workspace/TerminalTab.svelte`:
- Around line 109-111: The fire-and-forget terminal RPC calls (e.g., the onData
handler where _term.onData calls terminalWrite, and the other async RPC
invocations around the same area) can produce unhandled promise rejections when
sessions close or races occur; update each fire-and-forget call (specifically
the terminalWrite invocation inside _term.onData and the other two async RPC
calls referenced in the review) to append .catch(() => {}) so rejections are
swallowed while preserving the current non-await, latency-sensitive behavior.
- Around line 42-122: The async onMount flow can complete after onDestroy and
leak listeners/PTYs; add a local "mounted" (or "alive") boolean/flag set true at
start and set false in onDestroy, then after each await (listen(...) and
terminalCreate(...)) check the flag: if false then immediately cleanup any
resources returned (call the obtained _unlistenData/_unlistenExit functions,
dispose _webgl/_fit/_term/_ro/_themeObserver as appropriate) and do not call
terminalCreate or _term.write; also guard any subsequent uses of _term (e.g. in
the terminalWrite onData handler and ResizeObserver callback) so they no-op if
not mounted. Ensure you reference and manage the symbols _unlistenData,
_unlistenExit, listen, terminalCreate, _term, _webgl, _fit, _ro, _themeObserver
and onDestroy when implementing this check-and-cleanup flow.
---
Nitpick comments:
In `@src/lib/components/panels/TerminalPanel.svelte`:
- Around line 56-63: The shell-picker toggle button that flips the local
dropdownOpen state should expose that state to assistive tech by adding
aria-expanded={dropdownOpen} and aria-controls="shell-dropdown" (or another
unique id) to the button, and ensure the dropdown element rendered when
dropdownOpen is used has id="shell-dropdown" (and appropriate role like "menu"
or "listbox") so screen readers can associate the control with the popup; apply
the same aria-expanded/aria-controls change to the other toggle referenced (the
second dropdown toggle) and ensure ids are unique if both dropdowns exist.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 308fa548-9227-45cb-986f-67ba6ff4dce9
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.claude/settings.jsonpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/src/commands/mod.rssrc-tauri/src/commands/terminal.rssrc-tauri/src/lib.rssrc/lib/commands/terminal.jssrc/lib/components/panels/TerminalPanel.sveltesrc/lib/components/workspace/TerminalTab.sveltesrc/routes/app/+layout.svelte
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/components/panels/TerminalPanel.svelte (1)
31-33: ⚡ Quick winDon’t silently swallow shell discovery failures.
At Line 32,
catch {}hides actionable errors and makes “no shells found” indistinguishable from “failed to load shells.” Capture an error state so the UI can show a distinct failure message.Proposed patch
let shells = $state([]); + let shellsLoadError = $state(''); @@ onMount(async () => { - try { shells = await terminalListShells(); } catch {} + try { + shells = await terminalListShells(); + shellsLoadError = ''; + } catch (err) { + shellsLoadError = 'Failed to load shells'; + shells = []; + } }); @@ - {`#if` shells.length === 0} - <p class="px-3 py-2 text-xs text-muted-foreground">No shells detected</p> + {`#if` shellsLoadError} + <p class="px-3 py-2 text-xs text-destructive">{shellsLoadError}</p> + {:else if shells.length === 0} + <p class="px-3 py-2 text-xs text-muted-foreground">No shells detected</p>🤖 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 `@src/lib/components/panels/TerminalPanel.svelte` around lines 31 - 33, The current onMount block swallows errors from terminalListShells() (onMount, terminalListShells, shells) which prevents the UI from distinguishing "no shells" from "failed to load"; update the catch to capture the thrown error and set a dedicated error state (e.g., shellsError or shellsLoadError) and/or a boolean (e.g., shellsLoadingFailed) so the component can render a distinct failure message instead of silently doing nothing; ensure you still preserve any existing fallback for shells (e.g., empty array) but populate the new error state with the caught error (or a normalized message) for the template to read.
🤖 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 `@src/lib/components/panels/TerminalPanel.svelte`:
- Around line 113-123: The each block in TerminalPanel.svelte currently keys
shell rows by shell.program which may not be unique; update the key for the
{`#each` shells as shell (shell.program)} loop to a composite key combining
program, args and name (for example using shell.program + '-' +
shell.args.join(',') + '-' + shell.name) so each shell entry is unique; ensure
you reference the same shell variable used by openTerminal and the Terminal
component and handle cases where shell.args may be undefined (coerce to empty
array or string) when building the composite key.
In `@src/lib/stores/workspace.svelte.js`:
- Around line 87-90: renameTab currently accepts any input and can set a tab
title to blank/whitespace; update the store-level renameTab(tabId, title) to
validate and normalize the title before mutating tabs: trim the incoming title,
reject or fallback to a safe default (e.g., keep the existing tab.title or use
"Untitled") when the trimmed string is empty, and only assign the normalized
value to the matching tab found via tabs.find(t => t.id === id); ensure you
reference the renameTab function and the tabs collection when making this
change.
---
Nitpick comments:
In `@src/lib/components/panels/TerminalPanel.svelte`:
- Around line 31-33: The current onMount block swallows errors from
terminalListShells() (onMount, terminalListShells, shells) which prevents the UI
from distinguishing "no shells" from "failed to load"; update the catch to
capture the thrown error and set a dedicated error state (e.g., shellsError or
shellsLoadError) and/or a boolean (e.g., shellsLoadingFailed) so the component
can render a distinct failure message instead of silently doing nothing; ensure
you still preserve any existing fallback for shells (e.g., empty array) but
populate the new error state with the caught error (or a normalized message) for
the template to read.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c3aed7d-b3b7-487a-b125-2e92a56348cb
📒 Files selected for processing (4)
src-tauri/src/commands/terminal.rssrc/lib/components/panels/TerminalPanel.sveltesrc/lib/components/workspace/TerminalTab.sveltesrc/lib/stores/workspace.svelte.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/components/workspace/TerminalTab.svelte
- src-tauri/src/commands/terminal.rs
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
Chores