diff --git a/TODO.md b/TODO.md index 1babc85..1f03251 100644 --- a/TODO.md +++ b/TODO.md @@ -187,9 +187,10 @@ Pure tab-policy helpers that decide: ## Phase 3 — Centralize Editor/Tab Management (expanded) - [x] Implement shared tab policy layer for singleton tool tabs. - [x] Centralize singleton tab policies (`settings`, `tags`, `style`, `documentation`, `metadata-diff`, `site-validation`). -- Centralize transient/pin lifecycle rules (including promotion behavior). -- Remove duplicated tab-open logic from sidebars/nav components where possible. +- [x] Centralize post/media transient-vs-pinned open intent rules via shared entity-tab policy helper. +- [x] Remove duplicated tab-open logic from sidebars/nav/components for centralized policy-covered tab types. - [x] Ensure site-validation tab reopens with persisted last report and refreshes only on explicit validation trigger. +- [x] Centralize multi-instance tab opening policies for `chat`, `import`, and `git-diff` (file/commit resources). ### Editor Tab Management Variations (current analysis) 1. **Singleton, pinned tool tabs** @@ -253,4 +254,8 @@ Pure tab-policy helpers that decide: - [x] Phase 3 slice complete: added singleton tab policy helper and migrated menu/sidebar singleton tool openings to it. - [x] Phase 3 slice complete: site validation report is persisted per project and reloaded by `SiteValidationView`. - [x] Phase 3 slice complete: site validation view refreshes via explicit `bds:site-validation-updated` event (no auto-validate on mount). -- [ ] Next implementation slice: Phase 3 continuation (centralize non-singleton tab lifecycle/promotion rules). +- [x] Phase 3 continuation slice complete: added shared entity tab intent helper (`preview`/`pin`) for `post`/`media`. +- [x] Phase 3 continuation slice complete: migrated post/media open paths in Sidebar, Editor, LinkedMediaPanel, and Panel to shared entity-tab policy. +- [x] Phase 3 completion slice: centralized `chat` and `import` tab-open specs and migrated sidebar call sites. +- [x] Phase 3 completion slice: centralized `git-diff` file/commit ID/spec/open helpers and reused shared parsing in `TabBar`. +- [ ] Next implementation slice: Phase 4 (editor routing registry). diff --git a/src/renderer/components/Editor/Editor.tsx b/src/renderer/components/Editor/Editor.tsx index 7cab720..aeb91c7 100644 --- a/src/renderer/components/Editor/Editor.tsx +++ b/src/renderer/components/Editor/Editor.tsx @@ -21,6 +21,7 @@ import { SiteValidationView } from '../SiteValidationView'; import { AutoSaveManager, getContrastColor } from '../../utils'; import { InsertModal } from '../InsertModal'; import { AISuggestionsModal, AISuggestions } from '../AISuggestionsModal/AISuggestionsModal'; +import { openEntityTab } from '../../navigation/tabPolicy'; import { useI18n } from '../../i18n'; import './Editor.css'; @@ -1102,7 +1103,7 @@ const MediaEditor: React.FC<{ mediaId: string }> = ({ mediaId }) => { // Handle click on a post to navigate to it const handlePostClick = (postId: string) => { - openTab({ type: 'post', id: postId, isTransient: true }); + openEntityTab(openTab, 'post', postId, 'preview'); }; // Get unlinked posts for picker, filtered by search @@ -1680,12 +1681,12 @@ const Dashboard: React.FC = () => { onClick={() => { useAppStore.getState().setActiveView('posts'); useAppStore.getState().setSelectedPost(post.id); - useAppStore.getState().openTab({ type: 'post', id: post.id, isTransient: true }); + openEntityTab(useAppStore.getState().openTab, 'post', post.id, 'preview'); }} onDoubleClick={() => { useAppStore.getState().setActiveView('posts'); useAppStore.getState().setSelectedPost(post.id); - useAppStore.getState().openTab({ type: 'post', id: post.id, isTransient: false }); + openEntityTab(useAppStore.getState().openTab, 'post', post.id, 'pin'); }} > {post.title || tr('editor.untitled')} diff --git a/src/renderer/components/GitSidebar/GitSidebar.tsx b/src/renderer/components/GitSidebar/GitSidebar.tsx index 87d51ed..86868d3 100644 --- a/src/renderer/components/GitSidebar/GitSidebar.tsx +++ b/src/renderer/components/GitSidebar/GitSidebar.tsx @@ -1,5 +1,6 @@ import React, { useCallback, useEffect, useRef, useState } from 'react'; import { useAppStore } from '../../store'; +import { openGitDiffCommitTab, openGitDiffFileTab } from '../../navigation/tabPolicy'; import { useI18n } from '../../i18n'; import type { GitInitProgress, GitHistoryEntry, GitRemoteStateDto } from '../../../main/shared/electronApi'; import './GitSidebar.css'; @@ -125,9 +126,6 @@ export const GitSidebar: React.FC = () => { [tr], ); - const getDiffTabId = (filePath: string): string => `git-diff:${filePath}`; - const getCommitDiffTabId = (commitHash: string): string => `git-diff:commit:${commitHash}`; - const getActionProgressMessage = (action: 'fetch' | 'pull' | 'push' | 'prune-lfs' | 'commit'): string => { if (action === 'push') { return tr('gitSidebar.progress.pushingRemote'); @@ -156,22 +154,14 @@ export const GitSidebar: React.FC = () => { const openDiffTab = useCallback( (filePath: string, isTransient: boolean) => { - openTab({ - type: 'git-diff', - id: getDiffTabId(filePath), - isTransient, - }); + openGitDiffFileTab(openTab, filePath, isTransient ? 'preview' : 'pin'); }, [openTab], ); const openCommitDiffTab = useCallback( (commitHash: string, isTransient: boolean) => { - openTab({ - type: 'git-diff', - id: getCommitDiffTabId(commitHash), - isTransient, - }); + openGitDiffCommitTab(openTab, commitHash, isTransient ? 'preview' : 'pin'); }, [openTab], ); diff --git a/src/renderer/components/LinkedMediaPanel/LinkedMediaPanel.tsx b/src/renderer/components/LinkedMediaPanel/LinkedMediaPanel.tsx index e5b5722..caa22d1 100644 --- a/src/renderer/components/LinkedMediaPanel/LinkedMediaPanel.tsx +++ b/src/renderer/components/LinkedMediaPanel/LinkedMediaPanel.tsx @@ -11,6 +11,7 @@ import React, { useState, useEffect, useCallback } from 'react'; import { useAppStore, MediaData } from '../../store'; +import { openEntityTab } from '../../navigation/tabPolicy'; import { showToast } from '../Toast'; import { useI18n } from '../../i18n'; import './LinkedMediaPanel.css'; @@ -197,7 +198,7 @@ export const LinkedMediaPanel: React.FC = ({ // Handle click on media item to open media viewer const handleMediaClick = (mediaId: string) => { - useAppStore.getState().openTab({ type: 'media', id: mediaId, isTransient: true }); + openEntityTab(useAppStore.getState().openTab, 'media', mediaId, 'preview'); }; // Get thumbnail URL for a media item diff --git a/src/renderer/components/Panel/Panel.tsx b/src/renderer/components/Panel/Panel.tsx index 5c1be25..708a9e3 100644 --- a/src/renderer/components/Panel/Panel.tsx +++ b/src/renderer/components/Panel/Panel.tsx @@ -1,6 +1,7 @@ import React, { useEffect, useMemo, useRef, useState } from 'react'; import { useAppStore } from '../../store'; import type { TaskProgress } from '../../../main/shared/electronApi'; +import { openEntityTab } from '../../navigation/tabPolicy'; import { useI18n } from '../../i18n'; import './Panel.css'; @@ -296,7 +297,7 @@ export const Panel: React.FC = () => { } const handlePostLinkClick = (postId: string) => { - openTab({ type: 'post', id: postId, isTransient: false }); + openEntityTab(openTab, 'post', postId, 'pin'); setSelectedPost(postId); setActiveView('posts'); }; diff --git a/src/renderer/components/Sidebar/Sidebar.tsx b/src/renderer/components/Sidebar/Sidebar.tsx index 7843804..acc3369 100644 --- a/src/renderer/components/Sidebar/Sidebar.tsx +++ b/src/renderer/components/Sidebar/Sidebar.tsx @@ -8,7 +8,7 @@ import { scrollToSettingsSection, SettingsCategory } from '../SettingsView/Setti import { scrollToTagsSection, TagsCategory } from '../TagsView'; import { activateSidebarSection } from '../../navigation/sectionActivation'; import { getPersistedSidebarSection, setPersistedSidebarSection } from '../../navigation/sidebarUiPersistence'; -import { openSingletonToolTab } from '../../navigation/tabPolicy'; +import { openChatTab, openEntityTab, openImportTab, openSingletonToolTab } from '../../navigation/tabPolicy'; import type { SidebarView } from '../../navigation/sidebarViewRegistry'; import { useI18n } from '../../i18n'; import './Sidebar.css'; @@ -805,11 +805,11 @@ const PostsList: React.FC = ({ mode, isActive }) => { // Click handlers for tabs const handlePostClick = (postId: string) => { - openTab({ type: 'post', id: postId, isTransient: true }); + openEntityTab(openTab, 'post', postId, 'preview'); }; const handlePostDoubleClick = (postId: string) => { - openTab({ type: 'post', id: postId, isTransient: false }); + openEntityTab(openTab, 'post', postId, 'pin'); }; return ( @@ -1104,11 +1104,11 @@ const MediaList: React.FC = () => { }; const handleMediaClick = (mediaId: string) => { - openTab({ type: 'media', id: mediaId, isTransient: true }); + openEntityTab(openTab, 'media', mediaId, 'preview'); }; const handleMediaDoubleClick = (mediaId: string) => { - openTab({ type: 'media', id: mediaId, isTransient: false }); + openEntityTab(openTab, 'media', mediaId, 'pin'); }; // Determine which media to display @@ -1430,7 +1430,7 @@ const ChatList: React.FC = () => { const conversation = await window.electronAPI?.chat.createConversation(); if (conversation) { setConversations(prev => [conversation, ...prev]); - openTab({ type: 'chat', id: conversation.id, isTransient: false }); + openChatTab(openTab, conversation.id); } } catch (error) { console.error('Failed to create conversation:', error); @@ -1439,7 +1439,7 @@ const ChatList: React.FC = () => { }; const handleOpenChat = (conversationId: string) => { - openTab({ type: 'chat', id: conversationId, isTransient: false }); + openChatTab(openTab, conversationId); }; const handleDeleteChat = async (conversationId: string) => { @@ -1580,7 +1580,7 @@ const ImportList: React.FC = () => { const def = await window.electronAPI?.importDefinitions.create(); if (def) { setDefinitions(prev => [def, ...prev]); - openTab({ type: 'import', id: def.id, isTransient: false }); + openImportTab(openTab, def.id); } } catch (error) { console.error('Failed to create import definition:', error); @@ -1589,7 +1589,7 @@ const ImportList: React.FC = () => { }; const handleOpenDefinition = (definitionId: string) => { - openTab({ type: 'import', id: definitionId, isTransient: false }); + openImportTab(openTab, definitionId); }; const handleDeleteDefinition = async (e: React.MouseEvent, definitionId: string) => { diff --git a/src/renderer/components/TabBar/TabBar.tsx b/src/renderer/components/TabBar/TabBar.tsx index 8070fc8..f0c955b 100644 --- a/src/renderer/components/TabBar/TabBar.tsx +++ b/src/renderer/components/TabBar/TabBar.tsx @@ -1,22 +1,11 @@ import React, { useRef, useState, useEffect, useCallback } from 'react'; import { useAppStore, Tab } from '../../store'; +import { parseGitDiffTabId } from '../../navigation/tabPolicy'; import { useI18n } from '../../i18n'; import './TabBar.css'; const MAX_CHAT_TITLE_LENGTH = 18; -function getGitDiffResource(tabId: string): string { - return tabId.startsWith('git-diff:') ? tabId.slice('git-diff:'.length) : tabId; -} - -function getCommitHashFromGitDiffTabId(tabId: string): string | null { - const resource = getGitDiffResource(tabId); - if (!resource.startsWith('commit:')) { - return null; - } - return resource.slice('commit:'.length); -} - const getTabTitle = ( tab: Tab, postTitles: Map, @@ -27,8 +16,7 @@ const getTabTitle = ( tr: (key: string, vars?: Record) => string, ): string => { if (tab.type === 'git-diff') { - const filePath = getGitDiffResource(tab.id); - const commitHash = getCommitHashFromGitDiffTabId(tab.id); + const { resource: filePath, commitHash } = parseGitDiffTabId(tab.id); if (commitHash) { const commitTitle = commitTitles.get(commitHash); if (commitTitle) { @@ -380,7 +368,7 @@ export const TabBar: React.FC = () => { useEffect(() => { const commitHashes = tabs .filter((tab) => tab.type === 'git-diff') - .map((tab) => getCommitHashFromGitDiffTabId(tab.id)) + .map((tab) => parseGitDiffTabId(tab.id).commitHash) .filter((hash): hash is string => Boolean(hash)); if (commitHashes.length === 0 || !activeProject) { diff --git a/src/renderer/navigation/tabPolicy.ts b/src/renderer/navigation/tabPolicy.ts index ac48ff2..31efd28 100644 --- a/src/renderer/navigation/tabPolicy.ts +++ b/src/renderer/navigation/tabPolicy.ts @@ -14,6 +14,10 @@ export interface CanonicalTabSpec { isTransient: boolean; } +export type EntityTabType = 'post' | 'media'; +export type EntityTabOpenIntent = 'preview' | 'pin'; +export type GitDiffResourceOpenIntent = 'preview' | 'pin'; + const SINGLETON_TOOL_TAB_REGISTRY: Record = { settings: { type: 'settings', id: 'settings', isTransient: false }, tags: { type: 'tags', id: 'tags', isTransient: false }, @@ -33,3 +37,112 @@ export function openSingletonToolTab( ): void { openTab(getSingletonToolTabSpec(key)); } + +export function getEntityTabSpec( + type: EntityTabType, + id: string, + intent: EntityTabOpenIntent, +): CanonicalTabSpec { + return { + type, + id, + isTransient: intent === 'preview', + }; +} + +export function openEntityTab( + openTab: (tab: CanonicalTabSpec) => void, + type: EntityTabType, + id: string, + intent: EntityTabOpenIntent, +): void { + openTab(getEntityTabSpec(type, id, intent)); +} + +export function getChatTabSpec(conversationId: string): CanonicalTabSpec { + return { + type: 'chat', + id: conversationId, + isTransient: false, + }; +} + +export function openChatTab( + openTab: (tab: CanonicalTabSpec) => void, + conversationId: string, +): void { + openTab(getChatTabSpec(conversationId)); +} + +export function getImportTabSpec(definitionId: string): CanonicalTabSpec { + return { + type: 'import', + id: definitionId, + isTransient: false, + }; +} + +export function openImportTab( + openTab: (tab: CanonicalTabSpec) => void, + definitionId: string, +): void { + openTab(getImportTabSpec(definitionId)); +} + +export function getGitDiffFileTabId(filePath: string): string { + return `git-diff:${filePath}`; +} + +export function getGitDiffCommitTabId(commitHash: string): string { + return `git-diff:commit:${commitHash}`; +} + +export function getGitDiffFileTabSpec( + filePath: string, + intent: GitDiffResourceOpenIntent, +): CanonicalTabSpec { + return { + type: 'git-diff', + id: getGitDiffFileTabId(filePath), + isTransient: intent === 'preview', + }; +} + +export function getGitDiffCommitTabSpec( + commitHash: string, + intent: GitDiffResourceOpenIntent, +): CanonicalTabSpec { + return { + type: 'git-diff', + id: getGitDiffCommitTabId(commitHash), + isTransient: intent === 'preview', + }; +} + +export function openGitDiffFileTab( + openTab: (tab: CanonicalTabSpec) => void, + filePath: string, + intent: GitDiffResourceOpenIntent, +): void { + openTab(getGitDiffFileTabSpec(filePath, intent)); +} + +export function openGitDiffCommitTab( + openTab: (tab: CanonicalTabSpec) => void, + commitHash: string, + intent: GitDiffResourceOpenIntent, +): void { + openTab(getGitDiffCommitTabSpec(commitHash, intent)); +} + +export function parseGitDiffTabId(tabId: string): { resource: string; commitHash: string | null } { + const resource = tabId.startsWith('git-diff:') ? tabId.slice('git-diff:'.length) : tabId; + if (!resource.startsWith('commit:')) { + return { resource, commitHash: null }; + } + + return { + resource, + commitHash: resource.slice('commit:'.length), + }; +} diff --git a/tests/renderer/navigation/tabPolicy.test.ts b/tests/renderer/navigation/tabPolicy.test.ts index 0c0bd50..4f19dcd 100644 --- a/tests/renderer/navigation/tabPolicy.test.ts +++ b/tests/renderer/navigation/tabPolicy.test.ts @@ -1,5 +1,19 @@ import { describe, expect, it } from 'vitest'; -import { getSingletonToolTabSpec, openSingletonToolTab } from '../../../src/renderer/navigation/tabPolicy'; +import { + getChatTabSpec, + getEntityTabSpec, + getGitDiffCommitTabSpec, + getGitDiffFileTabSpec, + getImportTabSpec, + parseGitDiffTabId, + openChatTab, + getSingletonToolTabSpec, + openEntityTab, + openGitDiffCommitTab, + openGitDiffFileTab, + openImportTab, + openSingletonToolTab, +} from '../../../src/renderer/navigation/tabPolicy'; describe('tabPolicy', () => { it('provides canonical singleton tab specs', () => { @@ -22,4 +36,97 @@ describe('tabPolicy', () => { expect(captured).toEqual({ type: 'site-validation', id: 'site-validation', isTransient: false }); }); + + it('provides canonical entity tab spec for preview and pin intents', () => { + expect(getEntityTabSpec('post', 'post-1', 'preview')).toEqual({ + type: 'post', + id: 'post-1', + isTransient: true, + }); + + expect(getEntityTabSpec('media', 'media-1', 'pin')).toEqual({ + type: 'media', + id: 'media-1', + isTransient: false, + }); + }); + + it('opens entity tabs from canonical policy', () => { + let captured: { type: string; id: string; isTransient: boolean } | null = null; + const openTab = (tab: { type: string; id: string; isTransient: boolean }) => { + captured = tab; + }; + + openEntityTab(openTab, 'post', 'post-2', 'pin'); + + expect(captured).toEqual({ type: 'post', id: 'post-2', isTransient: false }); + }); + + it('provides canonical chat and import tab specs', () => { + expect(getChatTabSpec('conversation-1')).toEqual({ + type: 'chat', + id: 'conversation-1', + isTransient: false, + }); + + expect(getImportTabSpec('definition-1')).toEqual({ + type: 'import', + id: 'definition-1', + isTransient: false, + }); + }); + + it('opens canonical chat and import tabs', () => { + const opened: Array<{ type: string; id: string; isTransient: boolean }> = []; + const openTab = (tab: { type: string; id: string; isTransient: boolean }) => { + opened.push(tab); + }; + + openChatTab(openTab, 'conversation-2'); + openImportTab(openTab, 'definition-2'); + + expect(opened).toEqual([ + { type: 'chat', id: 'conversation-2', isTransient: false }, + { type: 'import', id: 'definition-2', isTransient: false }, + ]); + }); + + it('builds and parses git-diff file and commit tab specs', () => { + expect(getGitDiffFileTabSpec('posts/first.md', 'preview')).toEqual({ + type: 'git-diff', + id: 'git-diff:posts/first.md', + isTransient: true, + }); + + expect(getGitDiffCommitTabSpec('abc123def', 'pin')).toEqual({ + type: 'git-diff', + id: 'git-diff:commit:abc123def', + isTransient: false, + }); + + expect(parseGitDiffTabId('git-diff:posts/first.md')).toEqual({ + resource: 'posts/first.md', + commitHash: null, + }); + + expect(parseGitDiffTabId('git-diff:commit:abc123def')).toEqual({ + resource: 'commit:abc123def', + commitHash: 'abc123def', + }); + }); + + it('opens git-diff file and commit tabs from shared policy', () => { + const opened: Array<{ type: string; id: string; isTransient: boolean }> = []; + const openTab = (tab: { type: string; id: string; isTransient: boolean }) => { + opened.push(tab); + }; + + openGitDiffFileTab(openTab, 'posts/second.md', 'preview'); + openGitDiffCommitTab(openTab, 'def456', 'pin'); + + expect(opened).toEqual([ + { type: 'git-diff', id: 'git-diff:posts/second.md', isTransient: true }, + { type: 'git-diff', id: 'git-diff:commit:def456', isTransient: false }, + ]); + }); });