Conversation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add shared styles: _variables.scss, _mixins.scss, _index.scss - Centralize spacing scale ($spacing-xs to $spacing-2xl) - Centralize border radius ($radius-sm to $radius-xl) - Extract color variables ($color-success, $color-warning, $color-danger) - Create reusable mixins: flex-row, flex-center, form-input, interactive-card, toolbar-btn, btn-success, full-overlay, etc. - Migrate all 4 component SCSS files to use shared styles - No visual changes - pure refactor for maintainability Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create docs/DESIGN.md with SCSS architecture overview - Update CLAUDE.md with styles section and structure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update pane editor styling - Improve split preview appearance - Refine workspace list and editor components - Extend SCSS variables and mixins - Add session documentation and mockups Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- CI: Build check on push to dev and PRs to main - Release: Auto npm publish on merge to main Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- CONTRIBUTING: PRs go to dev, not main - README: Add Contributing section Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add workspaceId to recovery tokens for reliable detection after Tabby restart - Update isWorkspaceAlreadyOpen() with two-strategy detection: 1. Check recoveryToken.workspaceId (restored tabs) 2. Check profile ID (freshly opened tabs) - Add waitForTabbyReady() to ensure Tabby finishes recovery before launching - Remove per-pane title field, use workspace name for all tabs - Simplify tab title handling with disableDynamicTitle Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Document all issues found during code review: - 3 critical issues (memory leak, race condition, unsafe casts) - 5 moderate issues (DRY violations, error handling, caching) - 6 minor issues (docs, naming, performance) Includes fix recommendations and priority list for next release. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CR-001: Remove misleading ngOnDestroy from StartupCommandService - Service is module-level singleton, ngOnDestroy was never called - Added JSDoc documenting intentional singleton behavior - Removed unused Subscription variable CR-002: Fix race condition in shell initialization - Added timeout(2000) to prevent infinite wait - Added catchError for graceful fallback on timeout/error - Removed switchMap (was incorrect usage) - Extracted sendCommand helper to reduce duplication - Added setTabTitle helper method CR-003: Add type guards for workspace detection - Added RecoveryTokenWithWorkspace interface - Created type-safe getRecoveryWorkspaceId helper - Use startsWith + endsWith instead of includes() for strict matching - Added profilePrefix constant for clarity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix memory leak in StartupCommandService (CR-001) - Fix race condition in shell initialization (CR-002) - Add type-safe recovery token handling (CR-003) - Extract generic walkTree utility for DRY (CR-004) - Consolidate button styles with shared mixins (CR-005) - Add proper error handling with exceptions (CR-006) - Add TTL-based profile cache invalidation (CR-007) - Use OnPush change detection strategy (CR-008) - Add JSDoc documentation to public APIs (CR-009) - Extract magic numbers to SCSS variables (CR-010) - Add deepClone<T>() utility function (CR-014) - Migrate SCSS from @import to @use/@forward (CR-015) - Remove unused SCSS mixins (CR-016) - Delete CODE_REVIEW.md (all issues resolved) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace JSON.parse(JSON.stringify()) with deepClone() utility for consistency across the codebase. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Parent component mutates objects instead of creating new references, so OnPush strategy prevented UI updates until clicking elsewhere. Also added Angular CD guidelines to CLAUDE.md. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
After deleting a workspace, focus was lost and user couldn't interact with inputs until alt-tab. Root cause: native confirm() dialog takes focus away from Electron renderer and doesn't restore it properly. Solution: Use ng-bootstrap's NgbModal for in-app confirmation dialog that stays within Angular's focus management. Changes: - Add DeleteConfirmModalComponent for delete confirmation - Replace confirm() with NgbModal in workspaceList - Add @ng-bootstrap/ng-bootstrap@14 as devDependency - Add workspaceBackground.service.ts - Various component improvements and refactoring Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- S1 "Tight & Sharp" UI redesign - Tab bar navigation, inline pane editor - Bug fixes (focus, change detection) - Updated screenshots Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@claude please review this PR |
|
@claude review this PR |
There was a problem hiding this comment.
Pull request overview
Release preparation for v0.2.0, centered on the S1 “Tight & Sharp” UI redesign and related reliability fixes (startup workspace detection, command/title handling), plus documentation/build pipeline updates.
Changes:
- Introduces a shared SCSS design system (variables + mixins) and migrates components to the new styling/layout.
- Improves workspace startup/recovery behavior (workspaceId propagation, duplicate detection, startup command handling).
- Adds new UI flows (tabbed workspace navigation, inline pane editor, delete confirmation modal) and updates release/CI/docs.
Reviewed changes
Copilot reviewed 39 out of 42 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles/_variables.scss | Adds shared spacing/radius/color/z-index/transition variables for the new S1 design system. |
| src/styles/_mixins.scss | Adds reusable layout/form/button/overlay mixins used across components. |
| src/styles/_index.scss | Provides a single forwarding entry point for shared SCSS. |
| src/services/workspaceEditor.service.ts | Refactors workspace CRUD, profile caching, recovery token generation (workspaceId), and cloning. |
| src/services/workspaceBackground.service.ts | Adds dynamic CSS injection for per-workspace tab backgrounds. |
| src/services/startupCommand.service.ts | Refactors startup command sending logic and introduces timeout-based readiness handling. |
| src/providers/toolbar.provider.ts | Adds Tabby-ready wait + duplicate workspace detection on startup; initializes background service. |
| src/models/workspace.model.ts | Extends workspace model (background presets), removes pane title, adds deepClone helper. |
| src/index.ts | Registers new services/components (background + delete modal). |
| src/components/workspaceList.component.ts | Implements tab-bar navigation, modal delete confirmation, and state synchronization improvements. |
| src/components/workspaceList.component.scss | Restyles workspace list into S1 tab bar + content panel layout. |
| src/components/workspaceList.component.pug | Replaces vertical list with tab bar UI and editor content panel. |
| src/components/workspaceEditor.component.ts | Adds background picker logic, refactors tree operations, improves selection/editor state handling. |
| src/components/workspaceEditor.component.scss | Implements S1 editor layout, dropdown triggers, preview toolbar, and action button styling. |
| src/components/workspaceEditor.component.pug | Converts editor UI into sectioned layout with toolbar + inline pane editor + background picker. |
| src/components/splitPreview.component.ts | Adjusts selection/edit behavior and resets context menu state on split changes. |
| src/components/splitPreview.component.scss | Restyles split preview using shared variables/mixins; updates context menu styling. |
| src/components/splitPreview.component.pug | Removes edit icon/dblclick flow and simplifies events. |
| src/components/paneEditor.component.ts | Converts pane editor from modal workflow to inline panel with a close event. |
| src/components/paneEditor.component.scss | Restyles pane editor as inline details panel (S1). |
| src/components/paneEditor.component.pug | Removes modal overlay markup and switches to inline configuration UI. |
| src/components/deleteConfirmModal.component.ts | Adds NgbModal-based delete confirmation dialog. |
| scripts/build-prod.js | Uses explicit webpack-cli path for production builds. |
| scripts/build-dev.js | Uses explicit webpack-cli path for dev builds. |
| package.json | Bumps version to 0.2.0 and adds ng-bootstrap dev dependency. |
| package-lock.json | Updates lockfile dependencies (but currently has version metadata mismatch). |
| mockups/v06-tabbed.html | Adds UI mockup for tabbed interface exploration. |
| mockups/shared/base.css | Adds shared mockup styling baseline. |
| mockups/s1-tight-sharp.html | Adds S1 style mockup reference. |
| mockups/index.html | Adds mockup index/selector page. |
| docs/SESSION-2026-01-14-S1-DESIGN.md | Adds design implementation session notes. |
| docs/DESIGN.md | Adds design system documentation (currently out of sync with implementation). |
| TODO.md | Updates roadmap/bug list with recent work and status. |
| README.md | Updates feature list, screenshots, quick start, and contributing info (some details now out of sync with behavior). |
| CONTRIBUTING.md | Clarifies PR target branch workflow (dev vs main). |
| CLAUDE.md | Updates repo structure/style guidance (currently out of sync with implementation). |
| CHANGELOG.md | Adds 0.2.0 release notes. |
| .github/workflows/release.yml | Adds npm publish workflow on main. |
| .github/workflows/ci.yml | Adds CI build workflow on dev + PRs to main. |
| .claude/settings.local.json | Updates local Claude settings allowlist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Mixin | Purpose | | ||
| |-------|---------| | ||
| | `flex-row($gap)` | Horizontal flex with gap | | ||
| | `flex-col($gap)` | Vertical flex with gap | | ||
| | `flex-center` | Center content | | ||
| | `flex-between` | Space between | | ||
| | `form-input($bg)` | Styled input field | | ||
| | `interactive-card($radius)` | Clickable card with hover/selected states | | ||
| | `toolbar-btn` | Small toolbar button | | ||
| | `btn-success` | Green success button | | ||
| | `icon-btn-opacity` | Icon with hover opacity | | ||
| | `full-overlay($z)` | Fixed fullscreen overlay | | ||
| | `dropdown-panel` | Dropdown with border/shadow | | ||
| | `text-ellipsis` | Truncate text with ellipsis | |
There was a problem hiding this comment.
The mixin list here doesn't match what's actually defined in src/styles/_mixins.scss (e.g., flex-row/flex-col/flex-between/interactive-card/icon-btn-opacity are not present, while icon-btn-sm exists). Please update the table to reflect the real exported mixins to avoid misleading documentation.
CLAUDE.md
Outdated
| Modular DRY SCSS architecture. All components import `@import '../styles/index'`. | ||
|
|
||
| - **Variables**: `$spacing-*`, `$radius-*`, `$color-*`, `$z-*`, `$transition-*` | ||
| - **Mixins**: `flex-row`, `form-input`, `interactive-card`, `toolbar-btn`, `btn-success`, etc. |
There was a problem hiding this comment.
This section says components import shared styles via "@import '../styles/index'" and references mixins (flex-row, interactive-card) that aren't defined in src/styles/_mixins.scss. Update this to the current "@use '../styles/index' as *;" pattern and the actual mixin names to keep the repo guide accurate.
| Modular DRY SCSS architecture. All components import `@import '../styles/index'`. | |
| - **Variables**: `$spacing-*`, `$radius-*`, `$color-*`, `$z-*`, `$transition-*` | |
| - **Mixins**: `flex-row`, `form-input`, `interactive-card`, `toolbar-btn`, `btn-success`, etc. | |
| Modular DRY SCSS architecture. All components load shared styles via `@use '../styles/index' as *;`. | |
| - **Variables**: `$spacing-*`, `$radius-*`, `$color-*`, `$z-*`, `$transition-*` | |
| - **Mixins**: Layout, form, card, and button patterns. See `src/styles/_mixins.scss` for the available mixins. |
README.md
Outdated
| - Works with any shell (Bash, Zsh, PowerShell, Nushell, etc.) | ||
| - **Visual split editor** - Design layouts inline, not in modal dialogs. Split horizontally, vertically, nest to any depth | ||
| - **Layout toolbar** - Select a pane, then split, add adjacent panes, or delete with toolbar buttons | ||
| - **Per-pane configuration** - Set profile, working directory, startup command, and custom title for each pane |
There was a problem hiding this comment.
README still claims per-pane "custom title" support, but WorkspacePane.title was removed and the pane editor UI no longer exposes a title field. Please update the feature list to match current functionality (profile/cwd/startup command only, or reintroduce title support).
| - **Per-pane configuration** - Set profile, working directory, startup command, and custom title for each pane | |
| - **Per-pane configuration** - Set profile, working directory, and startup command for each pane |
| createWorkspace(): void { | ||
| console.log('[L5 DEBUG] createWorkspace called, cachedProfiles:', this.cachedProfiles.length) | ||
| const defaultProfileId = this.cachedProfiles[0]?.id || '' | ||
| const workspace = createDefaultWorkspace() | ||
| this.setProfileForAllPanes(workspace.root, defaultProfileId) |
There was a problem hiding this comment.
There are leftover debug logs (e.g., "[L5 DEBUG] ...") in createWorkspace(). These will spam the console in production; please remove them before release.
docs/DESIGN.md
Outdated
| All component SCSS files import shared styles: | ||
|
|
||
| ```scss | ||
| @import '../styles/index'; |
There was a problem hiding this comment.
This usage example still shows Sass "@import". The codebase has switched to the module system (e.g., components now use "@use '../styles/index' as *;"). Update the example to match the actual pattern so new contributors don't copy the deprecated approach.
| @import '../styles/index'; | |
| @use '../styles/index' as *; |
README.md
Outdated
| 1. **Open settings** - Settings > TabbySpaces | ||
| 2. **Create workspace** - Click "New Workspace", name it | ||
| 3. **Design layout** - Click a pane to select it, use toolbar to split (horizontal/vertical) | ||
| 4. **Configure panes** - Hover over a pane, click the edit icon to set profile, cwd, startup command |
There was a problem hiding this comment.
Quick Start instructions mention "hover over a pane, click the edit icon", but the split preview UI no longer has an edit icon/double-click (pane edit is now triggered by clicking the pane / context menu). Update these steps so they reflect the current interaction model.
| 4. **Configure panes** - Hover over a pane, click the edit icon to set profile, cwd, startup command | |
| 4. **Configure panes** - Click a pane (or use its context menu) to set profile, cwd, startup command |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: halilc4 <6174523+halilc4@users.noreply.github.com>
[WIP] Release version 0.2.0 with UI redesign and bug fixes
|
@claude , sta kazes, da li je dobar ovaj PR? |
|
Claude finished @halilc4's task —— View job PR Review for v0.2.0Odličan PR! Ovo je kvalitetna implementacija S1 design sistema sa solidnom arhitekturom. ✅ StrengthsArchitecture & Code Quality:
UX Improvements:
Implementation Details:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 42 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/index.ts:22
- NgBootstrap is now used (NgbModal, NgbActiveModal, ngbAutofocus), but the module doesn’t import any NgBootstrap module. Without adding
NgbModule/NgbModalModuleto@NgModule.imports, Angular will fail DI forNgbModaland templates usingngbAutofocuswill error as an unknown directive.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| on: | ||
| push: | ||
| branches: [dev] | ||
| pull_request: | ||
| branches: [main] |
There was a problem hiding this comment.
CI is configured to run on pull_request only for main, but CONTRIBUTING says all PRs target dev. As written, PRs into dev won’t get CI checks (only pushes to the dev branch do). Consider changing pull_request.branches to include dev (and optionally main) so PRs are validated before merge.
| label Icon | ||
| .dropdown-trigger((click)='toggleIconDropdown()') | ||
| span.dropdown-icon([style.color]='workspace.color') | ||
| i.fas([class]='"fa-" + workspace.icon') |
There was a problem hiding this comment.
The icon class is built from workspace.icon, but Workspace.icon is optional. For existing configs where icon is missing, this will render fa-undefined and show a broken icon. Use a fallback (e.g. workspace.icon || "columns") similar to other templates.
| i.fas([class]='"fa-" + workspace.icon') | |
| i.fas([class]='"fa-" + (workspace.icon || "columns")') |
| "Bash(git log:*)", | ||
| "Bash(ls -la \"C:\\\\Users\\\\halil\\\\tabbyspaces\\\\screenshots\"\" 2>nul || echo \"Directory not found \")", | ||
| "Bash(dir:*)", | ||
| "Bash(cmd.exe /c start \"\" \"C:\\\\Program Files \\(x86\\)\\\\Tabby\\\\Tabby.exe\" --remote-debugging-port=9222)" | ||
| "Bash(cmd.exe /c start \"\" \"C:\\\\Program Files \\(x86\\)\\\\Tabby\\\\Tabby.exe\" --remote-debugging-port=9222)", | ||
| "Bash(ls:*)" |
There was a problem hiding this comment.
.claude/settings.local.json contains local-machine command allowlists and absolute Windows paths. This looks like developer-local tooling config and is likely not intended to be tracked in the release branch. Consider removing it from the repo and adding .claude/ (or at least settings.local.json) to .gitignore, similar to how .mcp.json is handled.
| const cloned = {} as T | ||
| for (const key in obj) { | ||
| if (Object.prototype.hasOwnProperty.call(obj, key)) { | ||
| cloned[key] = deepClone(obj[key]) | ||
| } |
There was a problem hiding this comment.
deepClone won’t type-check under strict: true because key is a string in a for..in loop and T doesn’t have an index signature, so cloned[key] / obj[key] is a TS error. Use key as keyof T (and type cloned as something like Record<string, unknown> during assignment) or constrain T to an indexable type (e.g., T extends Record<string, unknown>).
- #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>
Summary
Checklist
After merge
GitHub Actions will automatically publish to npm.
🤖 Generated with Claude Code