feat: adds floating terminal layout#2344
Conversation
|
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:
✨ 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 |
github-actions
Bot
added
size:L
adamt94
force-pushed
the
feat/floating-terminal
branch
from
c78fad9 to
df27669
Compare
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces a new feature (floating terminal layout) with new user-facing behavior and UI components. Additionally, unresolved review comments identify substantive bugs: the restore defaults functionality doesn't reset the new terminal layout setting, and height clamping may not work correctly when switching between layouts. You can customize Macroscope's approvability policy. Learn more. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
adamt94
force-pushed
the
feat/floating-terminal
branch
from
df27669 to
2343d3d
Compare
adamt94
force-pushed
the
feat/floating-terminal
branch
from
2343d3d to
4796964
Compare
adamt94
changed the title
- add terminal layout setting and contract schema - render the thread terminal as a floating dialog when enabled - switch package scripts to run under Bun
adamt94
force-pushed
the
feat/floating-terminal
branch
from
f5b32fe to
7954880
Compare
| settings.automaticGitFetchInterval, | ||
| settings.enableAssistantStreaming, | ||
| settings.sidebarThreadPreviewCount, | ||
| settings.terminalLayout, |
There was a problem hiding this comment.
Restore defaults omits terminalLayout from reset
Medium Severity
The changedSettingLabels memo correctly detects when terminalLayout differs from its default and includes "Terminal layout" in the list shown in the confirmation dialog. However, the restoreDefaults callback's updateSettings call does not include terminalLayout, so the setting is never actually reset. The user sees it promised as restored but it stays unchanged.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7954880. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟠 High
The diff adds terminalLayout detection to changedSettingLabels (lines 414-416), so users see "Terminal layout" in the reset confirmation dialog, but restoreDefaults omits terminalLayout from the updateSettings call. The setting appears resettable but silently persists when confirmed.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/settings/SettingsPanels.tsx around line 467:
The diff adds `terminalLayout` detection to `changedSettingLabels` (lines 414-416), so users see "Terminal layout" in the reset confirmation dialog, but `restoreDefaults` omits `terminalLayout` from the `updateSettings` call. The setting appears resettable but silently persists when confirmed.
Evidence trail:
apps/web/src/components/settings/SettingsPanels.tsx lines 414-416 (terminalLayout in changedSettingLabels) and lines 467-480 (updateSettings call omitting terminalLayout) at REVIEWED_COMMIT.
| onHeightChangeRef.current(clampedHeight); | ||
| }, []); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Medium components/ThreadTerminalDrawer.tsx:1034
When the layout prop changes from "floating" to "docked", the drawer height is not re-clamped to the new maximum. The effect at line 1034 only depends on [height, threadId], so it won't run when layout changes, leaving a height of 800px (valid for floating's 920px max) exceeding docked's 750px max.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/ThreadTerminalDrawer.tsx around line 1034:
When the `layout` prop changes from `"floating"` to `"docked"`, the drawer height is not re-clamped to the new maximum. The effect at line 1034 only depends on `[height, threadId]`, so it won't run when `layout` changes, leaving a height of 800px (valid for floating's 920px max) exceeding docked's 750px max.
Evidence trail:
apps/web/src/components/ThreadTerminalDrawer.tsx lines 55-56 (ratio constants: 0.75 and 0.92), line 64-68 (clampDrawerHeight function), lines 884-887 (maxHeightRatio derived from layout, ref updated but no effect triggered), lines 1034-1039 (effect depends on [height, threadId] only), lines 1086-1107 (window resize handler only fires on window resize, not layout change). No other effect in the file depends on `layout`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
There are 6 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
| return normalizeThreadTerminalState({ | ||
| terminalOpen: normalized.terminalOpen, | ||
| terminalHeight: normalized.terminalHeight, | ||
| terminalWidth: normalized.terminalWidth, |
There was a problem hiding this comment.
Missing width on persisted state
High Severity
Persisted per-thread terminal state from before terminalWidth can lack that field. selectThreadTerminalState returns it unchanged, and clampFloatingTerminalWidth has no non-finite fallback (unlike drawer height clamping), so floating mode can apply a NaNpx dialog width until a store write normalizes the record.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
|
|
||
| useEffect(() => { | ||
| const clampedHeight = clampDrawerHeight(height); | ||
| const clampedHeight = clampDrawerHeight(height, maxHeightRatioRef.current); |
There was a problem hiding this comment.
Layout switch skips height reclamp
Medium Severity
Drawer height is reclamped when height or threadId changes, but not when layout switches between docked and floating. After floating mode with a taller drawer, switching to docked can leave the panel taller than the 75% viewport cap until a separate height or window resize event runs.
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
| const MIN_DRAWER_HEIGHT = 180; | ||
| const MAX_DRAWER_HEIGHT_RATIO = 0.75; | ||
| const DEFAULT_MAX_DRAWER_HEIGHT_RATIO = 0.75; | ||
| const FLOATING_MAX_DRAWER_HEIGHT_RATIO = 0.92; |
There was a problem hiding this comment.
Floating height ignores dialog chrome
Medium Severity
Floating layout caps the drawer at 92% of viewport height, but the modal also adds a header and outer padding. The combined height can exceed the viewport, clipping or overflowing the centered dialog on shorter screens.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
| const clamped = clampFloatingTerminalWidth(terminalState.terminalWidth); | ||
| floatingWidthRef.current = clamped; | ||
| setFloatingWidth(clamped); | ||
| }, [terminalState.terminalWidth, threadId]); |
There was a problem hiding this comment.
Floating width not viewport reclamped
Medium Severity
Floating terminal width is clamped during drag and when persisted width changes, but there is no window resize handler. After shrinking the browser, a stored width above 97% of the new viewport can leave the dialog wider than the screen until the user drags again.
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
| onPointerMove={handleWidthResizePointerMove} | ||
| onPointerUp={handleWidthResizePointerEnd} | ||
| onPointerCancel={handleWidthResizePointerEnd} | ||
| /> |
There was a problem hiding this comment.
Centered modal breaks edge resize
Medium Severity
The floating terminal is centered with flexbox while width changes use edge-drag math that assumes a fixed anchor. Resizing from the left or right handle moves both edges symmetrically, so the handles do not track the pointer and width changes feel wrong.
Reviewed by Cursor Bugbot for commit 9090da8. Configure here.
1 participant
Summary
Adds an option to have the terminal in a floating window
Settings page
floating terminal
Note
Low Risk
UI and persisted client preferences only; terminal session behavior is unchanged aside from presentation and width/height limits.
Overview
Introduces a docked vs floating terminal layout as a client setting (
TerminalLayout), defaulting to docked.When floating is selected, the same
ThreadTerminalDraweris shown inside a centered modal (backdrop dismiss, header close control,role="dialog") with horizontal resize on both edges; width is stored per thread interminalStateStore(default 900px, clamped 400px–97% viewport) and synced on drag end. The drawer gains alayoutprop: floating mode uses a taller max height ratio (92% vs 75%) and drops the docked top border styling.Settings adds a Terminal layout select (included in “changed settings” / restore defaults). Chat header copy and aria labels reflect “terminal window” vs “terminal drawer”. Contracts and persistence tests pick up the new setting field.
Reviewed by Cursor Bugbot for commit 9090da8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add floating terminal layout option as an alternative to the docked drawer
TerminalLayouttype ('docked'|'floating') to client settings in settings.ts, defaulting to'docked'.'floating', the terminal renders as a centered modal dialog with a backdrop, close button, and left/right resize handles instead of the bottom drawer.📊 Macroscope summarized 9090da8. 8 files reviewed, 3 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues