Polish right panel UI and terminal scrollback - #114
Conversation
…mposition - Extract ChangeSectionHeader and ChangeRow components for improved reusability - Add icon buttons with tooltips for section actions (stage/unstage/discard all) - Enhance ChecksNoPr with branch info, refresh button, and improved empty states - Add test coverage for AddWorktreeDialog user-edited branch handling
- Introduce `DEFAULT_TERMINAL_SCROLLBACK_LINES` (50k), min (100), and max (100k) constants in shared terminal module - Add `clampTerminalScrollbackLines()` and `getTerminalScrollbackBufferMaxLength()` to enforce bounds and calculate buffer size - Update RingBuffer in both `pty.ts` and `sessionHost.ts` to accept configurable `maxLength` in constructor and via `setMaxLength()` - Sync scrollback setting from renderer to main process via `settings:sync` IPC, apply to all active PTY instances - Add daemon protocol support: `spawn` now accepts optional `bufferMaxLength`, new `setBufferMaxLength` request type - Update all terminal views (BigTerminalView, TerminalPanel, SetupPanel) to watch for scrollback changes and update xterm options live - Validate user input in SettingsEditor with proper clamping bounds
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic terminal scrollback configuration across the application, allowing users to customize scrollback limits which are clamped and dynamically applied to active PTYs, daemon sessions, and renderer terminal instances. It also refactors the git changes file list into modular components (ChangeRow and ChangeSectionHeader) with improved styling and accessibility, enhances the "No PR" checks view with branch information and refresh actions, and fixes a bug in the worktree dialog where a user-edited or Jira-derived branch name could be overwritten when remote branches finished loading. The review feedback suggests a safer parsing approach for the terminalScrollback setting in the IPC handler to prevent type pollution from non-number values.
| if (typeof values.terminalScrollback === 'number') { | ||
| values.terminalScrollback = clampTerminalScrollbackLines(values.terminalScrollback) | ||
| } |
There was a problem hiding this comment.
The current check only validates if values.terminalScrollback is strictly of type number. Since this is an IPC handler receiving data from the renderer, it is safer to defensively parse the value (e.g., if it is passed as a string) and ensure it is a finite number before clamping it. This prevents potential type pollution of mainSettings.terminalScrollback with non-number values (like strings or null).
if (values.terminalScrollback !== undefined && values.terminalScrollback !== null) {
const parsed = Number(values.terminalScrollback)
if (Number.isFinite(parsed)) {
values.terminalScrollback = clampTerminalScrollbackLines(parsed)
} else {
delete values.terminalScrollback
}
} else if (values.terminalScrollback === null) {
delete values.terminalScrollback
}There was a problem hiding this comment.
Pull request overview
Polishes the right-panel changes/checks UI, raises terminal scrollback defaults, and threads scrollback configuration through live xterm instances and PTY ring buffers (including the PTY daemon protocol). Also adds a regression test for Jira-derived branch preservation in AddWorktreeDialog.
Changes:
- Refactor
ChangeFileListandChecksSections/ChecksNoPrinto clearer sub-components with new icons, denser controls, and richer empty/no-PR states; matching CSS inchanges*.css/checks.css/dialogs.css; localized copy updates in all four locales. - Centralize terminal scrollback constants/clamping in
src/shared/terminal.ts, apply them in the UI store,SettingsEditor, and live in right-panel, setup, standalone, and big xterm instances (without remounting); syncterminalScrollbackto main via existingsettings:sync. - Make PTY/daemon ring buffers configurable (
RingBuffer(maxLength),setBufferMaxLength,setScrollbackBufferMaxLength,SpawnRequest.bufferMaxLength, new daemonsetBufferMaxLengthrequest), withBUFFER_MAX_LENGTHderived from the shared default; addAddWorktreeDialogtest for Jira branch preservation across late remote loads.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/terminal.ts | New shared scrollback constants, clamp, and char-buffer conversion. |
| src/renderer/store/ui/terminal.ts | Clamp persisted/new scrollback values using shared helper. |
| src/renderer/components/Settings/SettingsEditor.tsx | Use shared min/max/clamp and update draft after blur. |
| src/renderer/components/Right/terminalCache.ts | Set scrollback on creation; add updateScrollbackAllTerminals. |
| src/renderer/components/Right/useTerminalLifecycle.ts | Apply scrollback on mount and on store changes. |
| src/renderer/components/Right/TerminalPanel.tsx | Live-update standalone terminal scrollback. |
| src/renderer/components/Right/SetupPanel.tsx | Apply scrollback to setup terminals on mount/changes. |
| src/renderer/components/Center/bigTerminalCache.ts | Set scrollback on reuse; add updateScrollbackAllBigTerminals. |
| src/renderer/components/Center/BigTerminalView.tsx | Subscribe to scrollback changes for big terminals. |
| src/renderer/App.tsx | Include terminalScrollback in settings:sync payload. |
| src/main/ipc.ts | Default/clamp terminalScrollback; push to PTY service on change. |
| src/main/services/pty.ts | Configurable RingBuffer; setScrollbackBufferMaxLength. |
| src/main/services/ptyDaemon/protocol.ts | Default buffer from shared; new SpawnRequest.bufferMaxLength and setBufferMaxLength request. |
| src/main/services/ptyDaemon/sessionHost.ts | RingBuffer takes max; live resize; per-host setter. |
| src/main/services/ptyDaemon/socketServer.ts | Forward spawn buffer max; handle setBufferMaxLength. |
| src/main/services/ptyDaemon/client.ts | Spawn passes buffer max; new client method. |
| src/main/services/ptyDaemon/adapter.ts | Size local/daemon buffers from current setting; best-effort daemon update. |
| src/main/services/ptyDaemon/tests/{protocol,sessionHost}.test.ts | Updated for derived default and explicit RingBuffer size. |
| src/renderer/components/Right/ChangeFileList.tsx | Refactor into ChangeSectionHeader/ChangeRow, use real buttons + icons. |
| src/renderer/components/Right/ChangesView.tsx | Use icon components for generate/refresh/pull; add type="button". |
| src/renderer/components/Right/useChangesActions.ts | Propagate staged flag to discard confirm payload. |
| src/renderer/components/Right/ChecksView.tsx | Derive branch name; pass refresh/branch/updated props to ChecksNoPr. |
| src/renderer/components/Right/ChecksSections.tsx | Add icon to ActionButton; richer no-checks/no-PR layouts. |
| src/renderer/styles/{changes,changes-controls,checks,dialogs}.css | Styles for new controls, empty blocks, no-PR card, Jira card grid. |
| src/renderer/locales/{en,id,ja,zh}/right.json | Updated noChecks/noPrHint; new noChecksHint/allChangesStaged. |
| src/renderer/components/Sidebar/AddWorktreeDialog.tsx | Move userEdited preservation into reducer; drop useRef mirror. |
| src/renderer/components/Sidebar/tests/AddWorktreeDialog.test.tsx | New test for Jira branch preservation across late remote load. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Layers touched
src/main/) — services, IPC handlerssrc/preload/) — context bridge APIsrc/renderer/) — components, stores, libApp.css)Changes
Sidebar / Center / Right panel:
Stores (
projects.ts/sessions.ts/ui.ts):Services (
git.ts/claude.ts/github.ts/pty.ts):IPC (
main/ipc.ts→preload/index.ts→lib/ipc.ts):terminalScrollbackthrough the existing settings sync path and update PTY buffer limits when it changes.How to test
npm run typechecknpm test -- src/main/services/ptyDaemon/__tests__/protocol.test.ts src/main/services/ptyDaemon/__tests__/sessionHost.test.tsnpm test -- src/renderer/components/Sidebar/__tests__/AddWorktreeDialog.test.tsxScreenshots
N/A
Checklist
yarn devnpm run typecheck