Sign up
View stargazersSign up

fix(web): terminal controls stop covering terminal text#19073

closed4
main
GitHub fork: fix/web-terminal-mobile-controls
opened21d agobyRrywible
1 files changed
View on GitHubTwo-way sync enabledPR last imported 14d ago

Problem

With a single terminal open, the split/new/close controls floated as an absolute overlay in the top-right corner of the terminal and covered the end of the first output row. Long prompts, input, and the caret disappeared under them, in the bottom drawer and in the right panel, on desktop widths and on phones. Fixes #19536.

Change

The single-terminal controls now sit in their own 22px row above the terminal instead of floating over it. The row reuses the exact button markup the multi-terminal sidebar header already renders, so the two layouts share one terminalActions element and cannot drift apart. The overlay-only pointer-events and z-index classes are gone, and the file is net smaller than on main.

Nothing changes for split layouts: the sidebar keeps its header and the controls move back into the row when only one terminal remains. The ghostty surface refits through its existing resize observer when the row appears or disappears.

Based on the original fix by @rywible in this PR, rebuilt to share the sidebar's toolbar instead of adding a second padded pill.

Verification

Checked in the web app against isolated dev state at 1280×800 and 412×800:

  • Drawer: the controls row ends at y=543 and the terminal canvas starts at y=543; the long first line is fully visible.
  • Right panel: row at y=52–73, terminal from y=74.
  • Split horizontally, then closed the new pane: the row disappears while the sidebar is shown and returns at full width afterwards, with the canvas refitting each time.
  • Phone width: the controls stay in their own row above the prompt.

vp run typecheck (apps/web), scoped lint and format checks, and vp test run src/components/ThreadTerminalDrawer.test.ts pass. No test was added: the only assertion possible here is static markup, which this repo does not test.

Not verified here: Electron desktop, macOS, Windows, iOS, and Android devices. This is a Linux headless browser run; the layout is plain CSS flex and does not depend on the platform.

Before (drawer, 1280×800): the overlay hides the end of the first line.

Before: controls cover the first terminal row

After (drawer, 1280×800):

After: controls in their own row above the terminal

After (right panel, 1280×800):

After: right panel controls row above the terminal

After (412px wide):

After: phone-width controls row above the prompt

Taken over with Claude Fable 5.1 (implementation), Claude Opus 5.5 and GPT-6 Sol (review) through T3 Code.

🤖 Generated with Claude Code

Checks

7 successful · 4 skipped
All checks passed
5e15128

Added 1 commit

  • fix(web): terminal controls stop covering text on phones7d91f98
CcoderabbitaiBot21d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6271e99e-dba6-4f94-8c8c-b0f2d1a63154
📥 Commits

Reviewing files that changed from the base of the PR and between 58731592a879bff57cfa360c84639c7c1ca0ed62 and 5e15128a0c4f2200d1b11501a218f32815596241.

📒 Files selected for processing: 1
  • apps/web/src/components/ThreadTerminalDrawer.tsx
🚧 Files skipped from review as they are similar to previous changes: 1
  • apps/web/src/components/ThreadTerminalDrawer.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The terminal drawer now uses one shared toolbar for split, new-terminal, and close-terminal actions. The single-terminal layout places the toolbar in a header row. The sidebar layout also renders the shared toolbar.

Changes: Terminal drawer layout

Layer / File(s)Summary
Shared terminal action toolbar
apps/web/src/components/ThreadTerminalDrawer.tsx		The shared toolbar retains the existing action handlers and disabled states. The single-terminal layout renders it in a header row, and the sidebar layout replaces its duplicate controls with the shared toolbar.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: arhammahajan

Merge Risk: ⚪ Minimal · up to 5e151

The controls move above the terminal text without changing their actions. No known merge-blocking issue remains.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Title checkPassedThe title clearly and concisely describes the main fix: terminal controls no longer cover terminal text.
Description checkPassedThe description includes the problem, implementation details, issue reference, verification steps, screenshots, test results, and unverified environments. It does not use a separate Scope and approval…
Full details: Description check

Explanation

The description includes the problem, implementation details, issue reference, verification steps, screenshots, test results, and unverified environments. It does not use a separate Scope and approval heading, but the issue reference and focused-fix context provide the required scope information.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

apps/web/src/components/ThreadTerminalDrawer.tsxlines 1619–1620Outdated · 7d91f98Open thread
……@@ -1616,7 +1616,7 @@ export default function ThreadTerminalDrawer({
16181618 {!hasTerminalSidebar && (
1619- <div className="pointer-events-none absolute right-2 top-2 z-20">
1619+ <div className="pointer-events-none relative z-20 flex shrink-0 justify-end px-2 pt-2 pb-1 sm:absolute sm:right-2 sm:top-2 sm:p-0">
16201620 <div className="pointer-events-auto inline-flex items-center overflow-hidden rounded-md border border-border/80 bg-background shadow-xs">
Aarhammahajanon lines 1619–162020d ago

The same overlap also happens at desktop widths (#19536). Dropping the sm: overrides fixes every width. Once the row is in normal layout flow, pointer-events-none, pointer-events-auto and z-20 are no longer needed. The terminal background on the row keeps the toolbar visually part of the terminal.

suggestion
1 <div className="flex shrink-0 justify-end bg-(--terminal-background) px-2 pt-2 pb-1">
2 <div className="inline-flex items-center overflow-hidden rounded-md border border-border/80 bg-background shadow-xs">

Verified in the web app at 1280×800:

  • Drawer and right panel: the toolbar row ends where the terminal starts, so a long first line stays fully visible.
  • Split: splitting moves the controls to the sidebar as before. Closing back to one terminal brings the row back.
  • Cost: about 36px of terminal height.
Rrywible20d ago

Thanks for the report and verification. Fixed in 58731592a8: the toolbar now stays in its own terminal-background row at every width, and the overlay-only pointer-event and z-index classes are removed.

I reproduced the desktop drawer overlap before the update, then verified the drawer and right panel at 1280×800, the horizontal split → single-terminal transition, and the drawer at 412×400. The row takes 35 CSS pixels in this build; controls end above the terminal canvas in each single-terminal view. Web typecheck and scoped lint/format checks passed.

The PR title, description, and before/after evidence now cover desktop as well as phone widths and link #19536. Updated evidence archive.

JJulius Marminge16d ago

Addressed in 5e15128. The single-terminal controls now share a normal-flow 22px row at every width, and the overlay-only pointer-event and stacking classes are removed. The PR body includes drawer, right-panel, split-to-single, and phone-width verification, with before/after evidence.

Added 1 commit

  • fix(web): terminal controls leave room for text at every width5873159

Added 1 commit

  • Merge remote-tracking branch 'origin/main' into fix/web-terminal-mobile-controls3da88dc
JJulius Marminge16d ago

Taking over this one. I'll rebuild the change against current main, verify it, and push to this branch.

Added 1 commit

  • fix(web): single-terminal controls get their own row5e15128