From 40892b9302277a4b0ba1754acc111c9beea7219f Mon Sep 17 00:00:00 2001 From: hugo Date: Sat, 21 Feb 2026 17:44:41 +0100 Subject: [PATCH] feat: phase 1b of refactorings --- TODO.md | 247 ++++++++++++++++++ .../components/ActivityBar/ActivityBar.tsx | 134 ++++------ src/renderer/navigation/activityBehavior.ts | 124 +++++++++ .../renderer/components/ActivityBar.test.tsx | 6 +- .../navigation/activityBehavior.test.ts | 93 +++++++ 5 files changed, 512 insertions(+), 92 deletions(-) create mode 100644 TODO.md create mode 100644 src/renderer/navigation/activityBehavior.ts create mode 100644 tests/renderer/navigation/activityBehavior.test.ts diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..950f98a --- /dev/null +++ b/TODO.md @@ -0,0 +1,247 @@ +# Activity/Sidebar/Editor Behavior Migration Plan + +## Goal +Create one shared behavior system for Activity Bar buttons, sidebar ownership, and editor/tab opening rules so all activities behave consistently by default. + +## Scope +- Renderer only (`src/renderer/**`) +- Activity Bar button behavior +- Sidebar switching/ownership +- Editor panel opening via tabs +- Tab policy for singleton tools (settings/tags/style/docs/etc.) + +## Non-goals +- Re-designing visuals +- Changing i18n copy beyond parity fixes +- Rewriting store architecture in one step + +--- + +## Current Behavior Exceptions (as-is) + +### A. Activity icon active-state exceptions +1. No known per-activity active-state exceptions in `ActivityBar`. + - All activity icons now use `sidebar-owner` active behavior. + +### B. Click behavior exceptions +1. No known per-activity click behavior exceptions in `ActivityBar`. + - Activity buttons now follow one shared posts-style switch/toggle pattern. +2. Remaining coupling is in sidebar section navigation (`SettingsNav`/`TagsNav`) and is planned for unified section activation API migration. + +### C. Sidebar content exceptions +1. **Posts/Pages** sidebars are mounted in wrappers with `display: none` when inactive. +2. **Media/Settings/Tags/Chat/Import/Git** sidebars are conditionally mounted. +3. **SettingsNav/TagsNav** perform section scrolling and can auto-open corresponding tabs before scroll. + +### D. Editor content exceptions +1. Editor is primarily tab-driven for tool views (`settings`, `tags`, `chat`, `import`, etc.). +2. Sidebar ownership (`activeView`) and editor content (active tab) are intentionally decoupled. +3. Some menu commands open tabs directly without harmonizing sidebar ownership. + +### E. API shape exceptions +1. Multiple places directly implement activity behavior logic instead of shared rules. +2. Open-tab semantics for singleton activities are duplicated across components. + +--- + +## Decisions (confirmed) + +1. **Active strategy is unified** + - Keep only `sidebar-owner`. + - Remove `sidebar-or-tab` special case. + - Consequence: Settings icon is active only when Settings owns the visible sidebar. + +2. **Click behavior is unified to Posts behavior** + - All activity buttons follow the same switch/toggle sidebar logic pattern. + - `import`, `settings`, and `tags` should not have unique click semantics. + - If a view needs to open/ensure a tab, that responsibility moves outside button-click special cases (e.g., explicit nav/editor actions), not activity click branching. + +3. **Sidebar/editor decoupling stays** + - Sidebar owner (`activeView`) and editor content (active tab) remain intentionally independent. + - Menu actions opening editors directly are valid and remain supported. + +4. **Editor tab management should be centralized next** + - Add shared tab policy/behavior layer to remove per-editor inconsistencies. + +5. **API-shape exception outlook** + - Most API-shape exceptions should disappear as behavior is centralized. + - A few explicit policy descriptors will still remain (not exceptions): e.g., tab kind, transience defaults, singleton keys. + +6. **Unified section activation API is required** + - Introduce one shared section activation API for editor/tool views. + - Sidebars should trigger section activation through that API instead of ad-hoc per-view scroll/open logic. + +7. **Sidebar mounting strategy is Variant B (conditional mount)** + - Mount only the currently active sidebar; unmount inactive sidebars. + - Rationale: simplest and most consistent behavior model now. + - Performance tuning can be revisited later if real bottlenecks appear. + - Given expected limited content sizes, this is acceptable as default. + +8. **Sidebar section actions always activate/open corresponding editor** + - Clicking a sidebar section/navigation action must always ensure the matching editor tab is active. + - If the editor tab is not open, it should be auto-opened first, then section activation should run. + +--- + +## Proposed Target Architecture + +## 1) Activity Registry (single source of truth) +A declarative registry describing each activity: +- `id` +- `sidebarView` +- `placement` (`top`/`bottom`) +- `labelKey` (i18n) +- `activeStrategy` (`sidebar-owner` only) +- `clickStrategy` (single unified strategy for all activity buttons) + +## 2) Shared Activity Behavior Engine +Pure functions that compute: +- `isActivityActive(snapshot, activityId)` +- `getActivityClickActions(snapshot, activityId)` + +No UI code inside this engine. Output is action descriptors consumed by UI components. + +## 3) Reusable Render Adapters +- `ActivityBar` renders from registry + icon map +- `Sidebar` picks section from registry/sidebar view mapping +- Later: `Editor` tool-tab mapping can also be declared instead of chained conditionals + +## 4) Shared Tab Policy Layer (new) +Pure tab-policy helpers that decide: +- singleton vs multi-instance +- transient vs pinned defaults +- promotion rules (transient -> pinned) +- when tab-open should happen from sidebar actions vs explicit editor/menu actions + +--- + +## Sidebar Mounting Variants: Behavior Tradeoffs + +### Variant A — Keep-mounted sidebars (hidden with CSS) +**How it works**: all sidebar components stay mounted; inactive ones are hidden. + +**Benefits** +- Preserves in-component UI state (expanded sections, scroll position, local filters) without extra persistence code. +- Fast switching (no remount cost). + +**Costs** +- Higher memory usage. +- More background effects/subscriptions unless each component guards itself. +- Risk of inactive sidebars still doing work. + +### Variant B — Conditionally mounted sidebars (mount/unmount) +**How it works**: only current sidebar component is mounted. + +**Benefits** +- Lower memory/CPU baseline. +- Clear lifecycle boundaries and fewer hidden side effects. +- Simpler mental model for ownership. + +**Costs** +- Local UI state resets unless persisted explicitly. +- Potentially more reload work when switching back. + +### Variant C — Hybrid (current) +**How it works**: some sidebars keep-mounted, others conditional. + +**Benefits** +- Tactical optimization where needed. + +**Costs** +- Inconsistent behavior and harder debugging. +- More “why is this one different?” maintenance overhead. + +### Recommendation for consistency +- Pick **A** or **B** globally. +- If preserving local sidebar UI state is important, pick **A**. +- If simplicity + lower background work is the priority, pick **B**. +- Avoid **C** long-term. + +--- + +## Phased Migration Plan + +## Phase 0 — Baseline Contract Tests (safety net) +- Add behavior tests that lock target UX for all activities. +- Remove/replace tests that validate deprecated exceptions. +- Outcome: refactor-safe baseline. + +## Phase 1 — Extract Activity Behavior Core (start here) +- [x] Add shared activity behavior module + typed activity registry for activity buttons. +- [x] Migrate `ActivityBar` to use shared behavior engine. +- [x] Keep visible behavior unchanged. +- [x] Add/adjust tests for helper + `ActivityBar` integration. + +## Phase 1b — Enforce Unified Activity Rules (new) +- [x] Remove `sidebar-or-tab` from activity behavior. +- [x] Make settings active-state `sidebar-owner` only. +- [x] Unify click behavior for `import/settings/tags` to posts pattern. +- [x] Update/expand contract tests for unified activity behavior and `ActivityBar` integration. + +## Phase 2 — Normalize Sidebar Mounting + Ownership Mapping +- Introduce a sidebar view registry mapping in one place. +- Pick one global mounting strategy (A or B) and apply to all sidebars. +- If needed, add explicit state persistence for remount-safe UX. +- Introduce unified section activation API and migrate `SettingsNav`/`TagsNav` to use it. + +## Phase 3 — Centralize Editor/Tab Management (expanded) +- Implement shared tab policy layer for all editor tabs. +- Centralize singleton tab policies (`settings`, `tags`, `style`, `documentation`, etc.). +- Centralize transient/pin lifecycle rules (including promotion behavior). +- Remove duplicated tab-open logic from sidebars/nav components where possible. + +### Editor Tab Management Variations (current analysis) +1. **Singleton, pinned tool tabs** + - `settings`, `tags`, `style`, `documentation` typically open as non-transient singleton tabs. +2. **Ephemeral/transient tabs with promotion** + - Post/media browsing flows can open transient tabs that become pinned on explicit user action (double-click or pin). +3. **Multi-instance tool tabs** + - `chat` and `import` create one tab per entity ID (conversation/definition), usually non-transient. +4. **Derived-ID utility tabs** + - `git-diff` uses structured IDs (`git-diff:` / commit variants), with special title derivation. +5. **Menu-opened utility tabs** + - `metadata-diff`, `site-validation`, and others are opened from menu commands with their own transience defaults. + +### Standardization target for tab management +- One registry describing for each tab type: + - `instanceMode`: `singleton | per-entity | per-resource` + - `defaultTransient`: `true | false` + - `promotable`: `true | false` + - `idStrategy`: canonical ID normalization/resolution + - `titleStrategy`: shared title resolver hooks + +## Phase 4 — Editor Routing Registry +- Replace long `if/else` tab-type rendering chain with declarative tab->component mapping. +- Keep special cases explicit (e.g., tab ID transforms such as git-diff commit/file parsing). + +## Phase 5 — Menu/Event Harmonization +- Route menu-driven view/tab actions through the same behavior layer where applicable. +- Reduce drift between keyboard/menu/toolbar interactions. + +## Phase 6 — Cleanup + Exception Review +- Reassess every exception in this file and decide keep/remove. +- Remove dead paths and duplicate helpers. +- Update tests and docs. + +--- + +## Decision Checklist (updated) +- [x] Remove `sidebar-or-tab`; use `sidebar-owner` only. +- [x] Unify activity click behavior to posts pattern for all activities. +- [x] Keep sidebar/editor decoupling. +- [x] Implement a unified section activation API for editor/tool sections. +- [x] Choose global sidebar mounting variant: **B conditional mount**. +- [x] Define sidebar action rule: section clicks always activate/open corresponding editor. +- [ ] Migrate existing `SettingsNav`/`TagsNav` auto-open + delayed scroll to the unified section activation API that enforces this rule. + +--- + +## Work Log +- [x] Documented migration strategy and current exception matrix. +- [x] Phase 1 slice complete: shared activity behavior extracted and wired into `ActivityBar`. +- [x] Decisions captured: unified active strategy, unified click behavior, editor/sidebar decoupling retained. +- [x] Decision captured: unified section activation API required. +- [x] Decision captured: sidebar mounting strategy Variant B (conditional mount). +- [x] Decision captured: sidebar section actions always activate/open corresponding editor. +- [x] Phase 1b complete: removed `sidebar-or-tab`, unified activity click behavior, and updated tests. +- [ ] Next implementation slice: Phase 2 (sidebar registry + unified section activation API migration). diff --git a/src/renderer/components/ActivityBar/ActivityBar.tsx b/src/renderer/components/ActivityBar/ActivityBar.tsx index 425bc6e..d1d88d4 100644 --- a/src/renderer/components/ActivityBar/ActivityBar.tsx +++ b/src/renderer/components/ActivityBar/ActivityBar.tsx @@ -1,6 +1,13 @@ import React from 'react'; import { useAppStore } from '../../store'; import { useI18n } from '../../i18n'; +import { + getActivityClickActions, + getActivityConfig, + isActivityActive, + type ActivityId, + type ActivitySnapshot, +} from '../../navigation/activityBehavior'; import './ActivityBar.css'; // Simple SVG icons @@ -58,121 +65,70 @@ const GitIcon = () => ( export const ActivityBar: React.FC = () => { const { t } = useI18n(); - const { activeView, setActiveView, sidebarVisible, toggleSidebar, openTab, tabs, activeTabId } = useAppStore(); - - // Check if settings tab is currently active - const isSettingsTabActive = tabs.some(t => t.type === 'settings' && t.id === activeTabId); - - // Check if settings view is active (either tab or sidebar) - const isSettingsActive = (activeView === 'settings' && sidebarVisible) || isSettingsTabActive; - - // Check if tags sidebar is active - const isTagsActive = activeView === 'tags' && sidebarVisible; - - // Check if chat sidebar is active (activeView === 'chat' and sidebar is visible) - const isChatActive = activeView === 'chat' && sidebarVisible; - - // Check if import sidebar is active - const isImportActive = activeView === 'import' && sidebarVisible; - const isGitActive = activeView === 'git' && sidebarVisible; - - // Handle view click - toggle sidebar if clicking on active view, otherwise switch view - const handleViewClick = (view: 'posts' | 'pages' | 'media' | 'chat' | 'git') => { - if (activeView === view && sidebarVisible) { - // Clicking on active view toggles sidebar off - toggleSidebar(); - } else if (activeView === view && !sidebarVisible) { - // Clicking on active view when hidden shows it - toggleSidebar(); - } else { - // Switching to a different view - ensure sidebar is visible - if (!sidebarVisible) { - toggleSidebar(); - } - setActiveView(view); - } + const { activeView, setActiveView, sidebarVisible, toggleSidebar, tabs, activeTabId } = useAppStore(); + const snapshot: ActivitySnapshot = { + activeView, + sidebarVisible, + tabs, + activeTabId, }; - const handleSettingsClick = () => { - // Toggle sidebar if settings is already active, otherwise switch to settings - if (activeView === 'settings' && sidebarVisible) { - toggleSidebar(); - } else { - // Open settings tab and show settings sidebar - openTab({ type: 'settings', id: 'settings', isTransient: false }); - setActiveView('settings'); - if (!sidebarVisible) { + const executeActivityClick = (activityId: ActivityId) => { + const actions = getActivityClickActions(snapshot, activityId); + + for (const action of actions) { + if (action.type === 'toggleSidebar') { toggleSidebar(); + } else if (action.type === 'setActiveView') { + setActiveView(action.view); } } }; - const handleTagsClick = () => { - // Toggle sidebar if tags is already active, otherwise switch to tags - if (activeView === 'tags' && sidebarVisible) { - toggleSidebar(); - } else { - openTab({ type: 'tags', id: 'tags', isTransient: false }); - setActiveView('tags'); - if (!sidebarVisible) { - toggleSidebar(); - } - } - }; - - const handleImportClick = () => { - if (activeView === 'import' && sidebarVisible) { - toggleSidebar(); - } else { - setActiveView('import'); - if (!sidebarVisible) { - toggleSidebar(); - } - } - }; + const getTitle = (activityId: ActivityId) => `${t(getActivityConfig(activityId).labelKey)} ${t('activity.toggleHint')}`; return (
@@ -180,16 +136,16 @@ export const ActivityBar: React.FC = () => {
diff --git a/src/renderer/navigation/activityBehavior.ts b/src/renderer/navigation/activityBehavior.ts new file mode 100644 index 0000000..87b5cc0 --- /dev/null +++ b/src/renderer/navigation/activityBehavior.ts @@ -0,0 +1,124 @@ +import type { Tab } from '../store/appStore'; + +export type ActivityId = 'posts' | 'pages' | 'media' | 'tags' | 'chat' | 'import' | 'git' | 'settings'; +export type SidebarView = 'posts' | 'pages' | 'media' | 'settings' | 'tags' | 'chat' | 'import' | 'git'; + +export interface ActivitySnapshot { + activeView: SidebarView; + sidebarVisible: boolean; + tabs: Tab[]; + activeTabId: string | null; +} + +type ActiveStrategy = 'sidebar-owner'; +type ClickStrategy = 'sidebar-toggle'; + +interface ActivityConfig { + id: ActivityId; + view: SidebarView; + labelKey: string; + placement: 'top' | 'bottom'; + activeStrategy: ActiveStrategy; + clickStrategy: ClickStrategy; +} + +const ACTIVITY_CONFIG: Record = { + posts: { + id: 'posts', + view: 'posts', + labelKey: 'activity.posts', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + pages: { + id: 'pages', + view: 'pages', + labelKey: 'activity.pages', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + media: { + id: 'media', + view: 'media', + labelKey: 'activity.media', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + tags: { + id: 'tags', + view: 'tags', + labelKey: 'activity.tags', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + chat: { + id: 'chat', + view: 'chat', + labelKey: 'activity.aiAssistant', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + import: { + id: 'import', + view: 'import', + labelKey: 'activity.import', + placement: 'top', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + git: { + id: 'git', + view: 'git', + labelKey: 'activity.sourceControl', + placement: 'bottom', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, + settings: { + id: 'settings', + view: 'settings', + labelKey: 'common.settings', + placement: 'bottom', + activeStrategy: 'sidebar-owner', + clickStrategy: 'sidebar-toggle', + }, +}; + +export const TOP_ACTIVITY_IDS: ActivityId[] = ['posts', 'pages', 'media', 'tags', 'chat', 'import']; +export const BOTTOM_ACTIVITY_IDS: ActivityId[] = ['git', 'settings']; + +export function getActivityConfig(activityId: ActivityId): ActivityConfig { + return ACTIVITY_CONFIG[activityId]; +} + +export function isActivityActive(snapshot: ActivitySnapshot, activityId: ActivityId): boolean { + const config = getActivityConfig(activityId); + return snapshot.activeView === config.view && snapshot.sidebarVisible; +} + +export type ActivityAction = + | { type: 'toggleSidebar' } + | { type: 'setActiveView'; view: SidebarView }; + +export function getActivityClickActions(snapshot: ActivitySnapshot, activityId: ActivityId): ActivityAction[] { + const config = getActivityConfig(activityId); + + if (snapshot.activeView === config.view) { + return [{ type: 'toggleSidebar' }]; + } + + const actions: ActivityAction[] = []; + + actions.push({ type: 'setActiveView', view: config.view }); + + if (!snapshot.sidebarVisible) { + actions.push({ type: 'toggleSidebar' }); + } + + return actions; +} diff --git a/tests/renderer/components/ActivityBar.test.tsx b/tests/renderer/components/ActivityBar.test.tsx index b70ee9a..089c1a5 100644 --- a/tests/renderer/components/ActivityBar.test.tsx +++ b/tests/renderer/components/ActivityBar.test.tsx @@ -15,7 +15,7 @@ describe('ActivityBar tags behavior', () => { }); }); - it('opens tags tab and switches sidebar to tags view when clicking Tags', () => { + it('switches sidebar to tags view when clicking Tags', () => { render( @@ -27,8 +27,8 @@ describe('ActivityBar tags behavior', () => { const state = useAppStore.getState(); expect(state.activeView).toBe('tags'); expect(state.sidebarVisible).toBe(true); - expect(state.activeTabId).toBe('tags'); - expect(state.tabs).toContainEqual({ type: 'tags', id: 'tags', isTransient: false }); + expect(state.activeTabId).toBeNull(); + expect(state.tabs).toEqual([]); }); it('toggles tags sidebar off when tags view is already active', () => { diff --git a/tests/renderer/navigation/activityBehavior.test.ts b/tests/renderer/navigation/activityBehavior.test.ts new file mode 100644 index 0000000..2ac5201 --- /dev/null +++ b/tests/renderer/navigation/activityBehavior.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from 'vitest'; +import { + getActivityClickActions, + isActivityActive, + type ActivityId, + type ActivitySnapshot, +} from '../../../src/renderer/navigation/activityBehavior'; + +function createSnapshot(overrides: Partial = {}): ActivitySnapshot { + return { + activeView: 'posts', + sidebarVisible: true, + activeTabId: null, + tabs: [], + ...overrides, + }; +} + +describe('activityBehavior', () => { + it('marks standard sidebar activities active only when they own the visible sidebar', () => { + const snapshot = createSnapshot({ activeView: 'posts', sidebarVisible: true }); + + expect(isActivityActive(snapshot, 'posts')).toBe(true); + expect(isActivityActive(snapshot, 'media')).toBe(false); + + const hiddenSidebar = createSnapshot({ activeView: 'posts', sidebarVisible: false }); + expect(isActivityActive(hiddenSidebar, 'posts')).toBe(false); + }); + + it('does not keep settings active from tab alone when another sidebar owns visibility', () => { + const snapshot = createSnapshot({ + activeView: 'posts', + sidebarVisible: true, + activeTabId: 'settings', + tabs: [{ type: 'settings', id: 'settings', isTransient: false }], + }); + + expect(isActivityActive(snapshot, 'settings')).toBe(false); + }); + + it('does not keep tags active from tab alone when another sidebar owns visibility', () => { + const snapshot = createSnapshot({ + activeView: 'posts', + sidebarVisible: true, + activeTabId: 'tags', + tabs: [{ type: 'tags', id: 'tags', isTransient: false }], + }); + + expect(isActivityActive(snapshot, 'tags')).toBe(false); + }); + + it('returns posts-style sidebar actions for settings/tags/import', () => { + const snapshot = createSnapshot({ activeView: 'posts', sidebarVisible: false }); + + expect(getActivityClickActions(snapshot, 'tags')).toEqual([ + { type: 'setActiveView', view: 'tags' }, + { type: 'toggleSidebar' }, + ]); + + expect(getActivityClickActions(snapshot, 'settings')).toEqual([ + { type: 'setActiveView', view: 'settings' }, + { type: 'toggleSidebar' }, + ]); + + expect(getActivityClickActions(snapshot, 'import')).toEqual([ + { type: 'setActiveView', view: 'import' }, + { type: 'toggleSidebar' }, + ]); + }); + + it('returns posts-style switch action when sidebar is already visible', () => { + const snapshot = createSnapshot({ activeView: 'posts', sidebarVisible: true }); + + expect(getActivityClickActions(snapshot, 'settings')).toEqual([ + { type: 'setActiveView', view: 'settings' }, + ]); + + expect(getActivityClickActions(snapshot, 'tags')).toEqual([ + { type: 'setActiveView', view: 'tags' }, + ]); + }); + + it('returns only sidebar actions for pure sidebar activities', () => { + const snapshot = createSnapshot({ activeView: 'chat', sidebarVisible: true }); + + expect(getActivityClickActions(snapshot, 'chat')).toEqual([{ type: 'toggleSidebar' }]); + }); + + it('supports all expected activity ids', () => { + const ids: ActivityId[] = ['posts', 'pages', 'media', 'tags', 'chat', 'import', 'git', 'settings']; + expect(ids).toHaveLength(8); + }); +});