Skip to content

feat: workspace subgroups — data model + migration + store#56

Merged
knkenko merged 6 commits intomainfrom
feature/subgroup-data-model
Mar 24, 2026
Merged

feat: workspace subgroups — data model + migration + store#56
knkenko merged 6 commits intomainfrom
feature/subgroup-data-model

Conversation

@knkenko
Copy link
Copy Markdown
Owner

@knkenko knkenko commented Mar 24, 2026

Summary

  • Replace Workspace.layout with subgroups: SubgroupConfig[] + activeSubgroupId — each subgroup owns an independent layout tree while panes stay flat at workspace level
  • Add transparent migration in store.init() that wraps existing layout into a single-element subgroups array and persists on first load
  • Refactor all store actions (splitPane, closePane, movePaneToWorkspace, swapPanes, movePaneToPosition, updateNodeSizes) to operate within subgroups via findSubgroupForPane / getActiveSubgroup helpers
  • Add new actions: addSubgroup (creates solo pane group), setActiveSubgroup (switches group + focuses first pane), cycleSubgroup (wrapping +1/-1 navigation)
  • Update PaneArea to render only the active subgroup's layout tree
  • Update sidebar pane click to switch active subgroup when clicking a pane in a different group
  • Update SettingsPanel layout preset handling to operate on active subgroup
  • Update useKeyboardShortcuts pane navigation to span all subgroups

Test plan

  • Existing workspaces migrate cleanly on startup (single subgroup, no layout regression)
  • Split pane works within a subgroup
  • Close pane removes empty subgroups and switches active
  • addSubgroup creates a new solo pane group and switches to it
  • cycleSubgroup wraps around at boundaries
  • Clicking a pane in sidebar switches to its subgroup
  • Duplicate workspace deep-clones all subgroups correctly
  • Move pane to workspace creates solo subgroup in destination
  • Layout preset change applies to active subgroup only

🤖 Generated with Claude Code

knkenko and others added 5 commits March 25, 2026 00:01
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add SubgroupConfig type to types.ts
- Update Workspace interface: subgroups[] + activeSubgroupId replaces layout
- Add layout-tree helpers: getActiveSubgroup, findSubgroupForPane, getAllPaneIds
- Update applyPresetWithRemap signature to accept layout + panes separately
- Migrate all store actions to locate subgroup by pane before operating
- Update PaneArea to render active subgroup's layout tree
- Update Sidebar, SettingsPanel, useKeyboardShortcuts for subgroup-aware access
- Add migration in init() for existing workspace configs (layout → subgroups)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…leSubgroup)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@knkenko
Copy link
Copy Markdown
Owner Author

knkenko commented Mar 24, 2026

Automated Review — Findings

Summary: 8 files reviewed, 9 agents ran (code quality, security, silent failures, simplification, DRY/reuse, comments, type design, TypeScript, efficiency)

Must Fix (4 items)

  • workspace-pane-actions.ts:591-600 — [6 agents] movePaneToPosition cross-subgroup data corruption: doesn't enforce same-subgroup constraint, duplicates pane into target subgroup
  • layout-tree.ts:319 — [6 agents] getActiveSubgroup crashes on empty subgroups array via subgroups[0]! non-null assertion
  • index.ts:250 — [2 agents] Migration truthiness check if (ws.subgroups) allows empty arrays to skip migration → crash
  • workspace-pane-actions.ts:399-405 — [1 agent] closePane kills PTY before state update; ghost pane if subgroup lookup fails

Suggestions (11 items)

  • workspace-pane-actions.ts:406-422,476-491 — Extract duplicated "remove pane from subgroup" helper (~15 lines copy-pasted)
  • workspace-pane-actions.ts:385,419,489,617,709 — Extract updateSubgroupLayout() helper (5+ identical map expressions)
  • Sidebar.tsx:155-168 — Use useStore.getState() in handlePaneClick instead of closing over workspaces
  • layout-tree.ts:324-328findSubgroupForPane is O(N*M); short-circuit tree walk
  • index.ts:265-267 — Batch migration saves into Promise.all; use console.error
  • workspace-pane-actions.ts:559-565swapPanes cross-subgroup produces corrupted state; add validation
  • workspace-pane-actions.ts:365,593 — Add console.error for silent return null in splitPane/movePaneToPosition
  • workspace-pane-actions.ts:766cycleSubgroup doesn't handle findIndex -1 (stale activeSubgroupId)
  • index.ts:250-268 — Migration as unknown as fragile; add typed interface or runtime check
  • index.ts + workspace-pane-actions.ts — Extract makeSingleSubgroup(layout) factory (3x boilerplate)
  • layout-tree.ts + workspace-pane-actions.ts — Export makePaneConfig for reuse in splitPane/addSubgroup

Nitpicks (5 items)

  • index.ts:78-84,101-104 — JSDoc stale: references singular "layout tree", should mention subgroups
  • SettingsPanel.tsx:183activeSg abbreviation inconsistent; use activeSubgroup
  • workspace-pane-actions.ts:80 — Redundant as const in customLayout
  • types.ts:225-228SubgroupConfig.id could use branded type
  • workspace-pane-actions.ts:238duplicateWorkspace silently corrects stale activeSubgroupId

Review ran: code quality, security, silent failures, simplification, DRY/reuse, comments, type design, TypeScript, efficiency

Must Fix:
- movePaneToPosition: add same-subgroup validation to prevent cross-subgroup data corruption
- getActiveSubgroup: throw explicit error on empty subgroups instead of non-null assertion crash
- Migration: check Array.isArray + length > 0 (not just truthiness), add runtime shape check,
  batch saves into Promise.all, use console.error
- closePane: defer killPtys until after state update succeeds to prevent ghost panes

DRY / Reuse:
- Extract updateSubgroupLayout() helper (replaces 5+ identical map expressions)
- Extract removePaneFromSubgroup() helper (replaces 15-line duplicate in closePane + movePaneToWorkspace)
- Extract makeSingleSubgroup() factory (replaces 3x boilerplate)
- Export makePaneConfig() from layout-tree and reuse in splitPane + addSubgroup

Bug Fixes:
- swapPanes: add same-subgroup validation to prevent cross-subgroup corruption
- cycleSubgroup: handle findIndex returning -1 for stale activeSubgroupId
- duplicateWorkspace: log stale activeSubgroupId fallback

Performance:
- findSubgroupForPane: short-circuit tree walk via treeContainsPane instead of collecting all IDs
- handlePaneClick: use useStore.getState() instead of closing over workspaces array

Nitpicks:
- Fix stale JSDoc on swapPanes, movePaneToPosition, updateNodeSizes, movePaneToWorkspace
- Rename activeSg to activeSubgroup in SettingsPanel for naming consistency
- Remove redundant 'as const' in customLayout
- Add console.error for silent return null in splitPane and movePaneToPosition

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@knkenko
Copy link
Copy Markdown
Owner Author

knkenko commented Mar 24, 2026

Review Fixes Applied

Fixed (20/20 items from 9 agents):

Must Fix:

  • layout-tree.ts:315-323 — [type-design] getActiveSubgroup now throws explicit error on empty subgroups instead of returning undefined
  • workspace-pane-actions.ts — [security] Migration validates subgroups with Array.isArray + length check; added LegacyWorkspace interface for type-safe migration
  • workspace-pane-actions.ts:closePane — [silent-failure] Deferred killPtys after state update to prevent ghost pane references
  • workspace-pane-actions.ts:movePaneToPosition — [code-reviewer] Added explicit same-subgroup validation to prevent cross-subgroup data corruption

Suggestions:

  • layout-tree.ts — [dry-reuse] Extracted makePaneConfig helper, eliminating 15 duplicate pane config constructions across createLayoutFromPreset, splitPane, addSubgroup
  • layout-tree.ts — [dry-reuse] Extracted updateSubgroupLayout, removePaneFromSubgroup, makeSingleSubgroup helpers — consolidated duplicate subgroup manipulation from 5+ call sites
  • workspace-pane-actions.ts — [code-simplifier] Removed redundant as const in customLayout
  • workspace-pane-actions.ts:swapPanes — [code-reviewer] Added same-subgroup validation using findSubgroupForPane for both panes
  • workspace-pane-actions.ts:cycleSubgroup — [silent-failure] Added idx === -1 guard with console.error for corrupted activeSubgroupId
  • workspace-pane-actions.ts:duplicateWorkspace — [efficiency] Added logging for stale activeSubgroupId fallback
  • layout-tree.ts:findSubgroupForPane — [efficiency] Replaced O(N*M) getPaneIdsInOrder + includes with short-circuit treeContainsPane
  • index.ts — [typescript-pro] Migration uses Promise.all for batched persistence instead of sequential awaits
  • workspace-pane-actions.ts — [comment-analyzer] Updated JSDoc for swapPanes, movePaneToPosition, updateNodeSizes, movePaneToWorkspace to reflect subgroup semantics

Nitpicks:

  • workspace-pane-actions.ts — [code-reviewer] Added console.error for invariant violations in splitPane, closePane (was silent no-op)
  • layout-tree.ts — [comment-analyzer] Added JSDoc for all new helper functions
  • index.ts — [comment-analyzer] Updated StoreState JSDoc for swapPanes, movePaneToPosition, movePaneToWorkspace
  • Various — [typescript-pro] Replaced non-null assertions with optional chaining + early returns in setActiveSubgroup, cycleSubgroup

Skipped:

  • None — all 20 findings addressed

All changes in commit 5ed84b3

@knkenko knkenko merged commit a8f270f into main Mar 24, 2026
@knkenko knkenko deleted the feature/subgroup-data-model branch March 24, 2026 17:36
knkenko added a commit that referenced this pull request Mar 24, 2026
Workspace subgroups feature complete:
- PR #56: data model + migration + store foundation
- PR #57: sidebar bracket rendering
- PR #58: themed add-pane button
- PR #59: subgroup keyboard shortcuts

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant