fix: unsynced line buffer on backspace or on keyboard type #645
Conversation
WalkthroughThe pull request refactors terminal input/output handling across backend and frontend. The backend removes output buffering, sending websocket data directly per chunk. The frontend introduces dedicated ref tracking for terminal instances, implements cleanup on deactivation and unmount, refines key handling for copy and toggle operations, and updates the API version timestamp. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![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
🧹 Nitpick comments (1)
view/app/terminal/utils/useTerminal.ts (1)
90-101: Consider consolidating cleanup responsibility.This cleanup effect duplicates logic already present in
terminal.tsx(lines 63-70 and 78-83). While it's safe due todestroyTerminalbeing idempotent, having cleanup in both the hook and the component creates maintenance burden.Consider choosing one owner for cleanup:
- Hook-owned cleanup (current approach here): The hook manages its own lifecycle internally
- Consumer-owned cleanup (terminal.tsx approach): The consumer explicitly calls
destroyTerminalHaving both is redundant. If the hook is designed to be self-cleaning, consumers shouldn't need additional cleanup effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/api/versions.json(1 hunks)api/internal/features/terminal/init.go(0 hunks)view/app/terminal/terminal.tsx(1 hunks)view/app/terminal/utils/useTerminal.ts(5 hunks)
💤 Files with no reviewable changes (1)
- api/internal/features/terminal/init.go
🧰 Additional context used
🪛 ast-grep (0.40.0)
view/app/terminal/utils/useTerminal.ts
[warning] 44-44: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: terminalRef.current.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (5)
api/api/versions.json (1)
6-6: LGTM!Version timestamp update is appropriate for tracking this release.
view/app/terminal/terminal.tsx (1)
63-83: LGTM - Cleanup lifecycle is well-structured.The dual cleanup approach (on deactivation and on unmount) correctly handles all terminal teardown scenarios. The implementation is safe because
destroyTerminalinuseTerminal.tsis idempotent—it checksterminalInstanceRef.currentbefore disposing, so multiple calls are harmless.view/app/terminal/utils/useTerminal.ts (3)
36-50: destroyTerminal is correctly idempotent.The implementation properly guards disposal with
terminalInstanceRef.currentcheck, making it safe to call multiple times. The empty dependency array is correct since only refs and setters are used.Regarding the static analysis warning on
innerHTML = '': this is a false positive. SettinginnerHTMLto an empty string to clear a container is safe—no user-controlled content is being inserted.
210-217: LGTM - Backend-driven echo is the correct approach.Forwarding all input to the backend and relying on backend echo for display synchronization is the right fix for the line buffer sync issues. This works well with the backend change that removed buffering in favor of per-chunk websocket writes.
232-233: LGTM - Dual tracking pattern is appropriate.Storing the terminal in both a ref and state serves distinct purposes: the ref enables synchronous access for cleanup (avoiding stale closure issues), while state triggers reactive updates for effects that depend on
terminalInstance.
| // Handle Ctrl+C or Cmd+C for copy (when there's a selection) | ||
| if (key === 'c' && (event.ctrlKey || event.metaKey) && !event.shiftKey) { | ||
| if (event.type === 'keydown') { | ||
| try { | ||
| const selection = term.getSelection(); | ||
| if (selection) { | ||
| navigator.clipboard.writeText(selection).then(() => { | ||
| term.clearSelection(); // Clear selection after successful copy | ||
| term.clearSelection(); | ||
| }); | ||
| return false; | ||
| } | ||
| } catch (error) { | ||
| console.error('Error in Ctrl+C handler:', error); | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Add error handling for clipboard write failure.
The navigator.clipboard.writeText() promise rejection is not caught. If clipboard access fails (e.g., due to permissions), the error will be silently swallowed and clearSelection() won't be called.
if (selection) {
- navigator.clipboard.writeText(selection).then(() => {
- term.clearSelection();
- });
+ navigator.clipboard.writeText(selection)
+ .then(() => {
+ term.clearSelection();
+ })
+ .catch((err) => {
+ console.error('Failed to copy to clipboard:', err);
+ });
return false;
}🤖 Prompt for AI Agents
In view/app/terminal/utils/useTerminal.ts around lines 189 to 204, the clipboard
write call uses navigator.clipboard.writeText(selection) without handling
promise rejections so failures will be silent and term.clearSelection() may not
run; wrap the clipboard write in a try/catch (or attach .catch) and ensure
term.clearSelection() runs in a finally block (or after both success and
failure) and still return false after handling the selection so the key event is
swallowed.
|
@coderabbitai: update description |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1069 @zhravan: I'm unable to update the PR description directly due to workflow restrictions. However, I've prepared a comprehensive description based on the code changes. Please copy and paste this into the PR description: IssueFixes terminal display sync issues where the line buffer becomes unsynced on backspace or keyboard input, causing the terminal display to not accurately reflect the actual terminal state. DescriptionThis PR addresses synchronization issues in the terminal by refactoring both backend output handling and frontend terminal lifecycle management. Backend Changes (api/internal/features/terminal/init.go)
Frontend Changes (view/app/terminal/utils/useTerminal.ts)
Frontend Changes (view/app/terminal/terminal.tsx)
Scope of Change
TestingManual testing confirms:
Developer Checklist
Reviewer Checklist
You can update the PR description by clicking the "Edit" button on the PR page and pasting this content. |
Issue
Link to related issue(s): #601
localhost_3000_self-host.webm
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.