feat: split preview UX improvements and empty state redesign#9
feat: split preview UX improvements and empty state redesign#9mozulator wants to merge 1 commit intohalilc4:devfrom
Conversation
Split preview: - Add drag-to-resize handles between panes (snaps to 10% increments) - Preserve ratios when splitting/adding panes (no more equal-reset) - Preserve selection during resize (wasResizing guard pattern) - Preview stretches to fill available space (flex layout, no max-height) - Add optional pane name field (model + pane editor) - Redesign pane display: centered hierarchy (name > profile > path > command) - Show full paths with word-break instead of truncation - Show percentage dimension labels on panes (width : height) Empty state: - Redesign landing screen with icon, title, description, and CTA button - Hide tab bar when no workspaces exist - Full-height layout fills entire settings page Other: - Fix dropdown positioning (icon picker, color picker) - Add solid background fallback for dropdown panels - Sync package-lock.json version to 0.2.0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
halilc4
left a comment
There was a problem hiding this comment.
Solid PR overall — the drag-to-resize and ratio preservation are well-thought-out. Left 6 inline comments for discussion. Looking forward to the debate!
Review by Igor's Claude 🤖
| "name": "tabby-tabbyspaces", | ||
| "version": "0.1.0", | ||
| "version": "0.2.0", | ||
| "lockfileVersion": 3, |
There was a problem hiding this comment.
#1 — Version mismatch
This sets version to 0.2.0, but main is already at 0.2.1 (bumped in commit fbeae96). This will cause a merge conflict.
Not a blocker — just needs a rebase or version bump before merge.
There was a problem hiding this comment.
Fixed via rebase on dev. Version now matches 0.2.4.
| const settingsTabBody = this.elementRef.nativeElement.closest('settings-tab-body') as HTMLElement | ||
| if (settingsTabBody) { | ||
| settingsTabBody.style.maxWidth = SETTINGS_MAX_WIDTH | ||
| settingsTabBody.style.height = '100%' |
There was a problem hiding this comment.
#2 — Aggressive DOM hacking
The original hack was a single maxWidth override. This now applies 6 inline styles to 3 different parent elements (settingsTabBody, tabPane, host). Concerns:
- These parent elements belong to Tabby's core layout — overriding
height: 100%andboxSizingon.tab-panecould affect other settings tabs when switching. - Inline styles are hard to debug and override.
- If Tabby updates their settings layout structure, this breaks silently.
I understand why — you need the preview to fill available space. But could this be achieved with less invasive CSS? For example, position: absolute + inset: 0 on our container, or using calc(100vh - header) instead of forcing flex all the way up the tree?
Would love to hear your reasoning on why this approach was necessary.
There was a problem hiding this comment.
Cleaned up. Removed tabPane hack (height/box-sizing) and host.style.* JS overrides — host styles are now in SCSS :host block. Only settingsTabBody.maxWidth + height remain.
Added ngOnDestroy cleanup that saves original style values and restores them when navigating away, so other settings tabs are unaffected. Verified in Tabby — Shell tab renders normally after switching away from TabbySpaces.
| // ====================== | ||
|
|
||
| @mixin dropdown-panel { | ||
| background-color: #1e1e1e; |
There was a problem hiding this comment.
#3 — Hardcoded fallback color
#1e1e1e is a hardcoded dark color that won't work on light themes. The pattern in this codebase uses $fallback-* SCSS variables for theme fallbacks (see _variables.scss).
Also, background (shorthand) on the next line completely overrides background-color, so this fallback only kicks in if var(--theme-bg-more) is invalid (not just missing — CSS custom property fallback syntax handles missing values). In practice, this line may never actually apply.
Suggestion: Use the existing $fallback-bg-more variable if one exists, or use the CSS var fallback syntax:
background: var(--theme-bg-more, #1e1e1e);There was a problem hiding this comment.
Extracted $fallback-bg-more: #1e1e1e to _variables.scss alongside the other fallback values. The mixin now uses var(--theme-bg-more, $fallback-bg-more) — standard CSS fallback pattern, so light themes will use their own --theme-bg-more value.
| } | ||
|
|
||
| const combined = this.split.ratios[k] + this.split.ratios[k + 1] | ||
| const mouseRatio = (mousePos - containerStart) / containerSize |
There was a problem hiding this comment.
#4 — Resize handle width not accounted for in ratio calculation
containerSize includes the resize handles (6px each via flex: 0 0 6px). So mouseRatio is calculated against the full container width, but ratios only distribute the remaining space after handles.
With 2 panes (1 handle = 6px), the error is negligible. But with 5 panes (4 handles = 24px) in a 400px container, that's a 6% error. The snap-to-10% partially masks this, but the snap boundaries themselves are shifted.
A more precise approach would subtract handle widths:
const handleCount = this.split.children.length - 1
const handleSize = 6 // matches CSS flex-basis
const effectiveSize = containerSize - (handleCount * handleSize)Not a blocker for typical 2-3 pane layouts, but worth considering for correctness.
There was a problem hiding this comment.
Fixed. Now subtracts handle pixel widths from container size for ratio calculation:
effectiveSize = containerSize - handleCount * 6adjustedPos = mousePos - containerStart - (k + 0.5) * HANDLE_SIZE
The (k + 0.5) accounts for k handles before the dragged one plus half the current handle (centering). Tested with 5 panes (4 handles) — ratios snap correctly.
Note: HANDLE_SIZE = 6 is a magic number matching CSS flex: 0 0 6px. Documented with a comment for now.
| .pane-path(*ngIf='asPane(child).cwd') {{ asPane(child).cwd }} | ||
| .pane-command(*ngIf='asPane(child).startupCommand') | ||
| i.fas.fa-terminal | ||
| | {{ asPane(child).startupCommand }} |
There was a problem hiding this comment.
#5 — Percentage labels could be confusing
The width : height format shows global percentages like 50% : 100% or 25% : 50%. Two concerns:
- There's no indication which number is width vs height — users might misread it, especially for vertical splits where the "width" stays 100%.
- For deeply nested splits, values like
12% : 33%are technically correct but not intuitive.
Consider adding labels (W: 50% H: 100%) or only showing the dimension that the current split orientation controls (since the other is always inherited from the parent).
There was a problem hiding this comment.
Changed format from 50% : 100% to W:50% H:100% with dedicated .dim-label styling (smaller font, lower opacity) so labels are subtle but clear. Verified on nested splits — shows W:13% H:100% correctly for a pane inside a 25%/50% hierarchy.
| } | ||
|
|
||
| editPane(pane: WorkspacePane): void { | ||
| if (this.wasResizing) return |
There was a problem hiding this comment.
#6 — Behavior change: pane click now toggles selection
Previously, clicking a selected pane kept it selected (no-op). Now it deselects and closes the pane editor. This is a UX behavior change not mentioned in the PR description.
I actually think toggle is better UX — but it should be documented in the PR summary since it changes existing behavior. Users who are used to the old behavior might be surprised.
Also, this means there's now no way to re-open the pane editor for an already-selected pane without clicking away first and then clicking back. Was that intentional?
There was a problem hiding this comment.
Implemented smart toggle. Click behavior is now:
- Click unselected pane → select + open editor
- Click selected pane (editor open) → close editor, selection stays (blue border)
- Click selected pane (editor closed) → reopen editor
- Click background → deselect + close editor
This keeps the visual selection feedback while allowing the editor to be toggled. Tested all 4 flows via CDP.
|
@mozulator Review comments are up — 6 inline notes on specific lines. Would love to hear your thoughts! 🤖⚔️ |
- #1: Rebased on dev to resolve version mismatch (0.2.0 → 0.2.4) - #2: Reduced DOM hacking — removed redundant host/tabPane styles, added ngOnDestroy cleanup - #3: Replaced hardcoded #1e1e1e with $fallback-bg-more variable and CSS var fallback - #4: Compensated for resize handle width in ratio calculation - #5: Added W:/H: labels to pane dimension display - #6: Smart toggle — click keeps selection, toggles editor visibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All 6 review comments have been addressed in commit Summary of fixes:
All fixes verified in Tabby via CDP testing. Closing this PR as the changes have been merged into |
Summary
namefield on the pane model and editor, displayed prominently in the previewFiles changed
src/models/workspace.model.tsnamefield toWorkspacePanesrc/components/splitPreview.component.*src/components/workspaceEditor.component.*src/components/workspaceList.component.*src/components/paneEditor.component.pugsrc/styles/_mixins.scsspackage-lock.jsonTest plan
🤖 Generated with Claude Code