diff --git a/apps/frontend/src/main/__tests__/ipc-handlers.test.ts b/apps/frontend/src/main/__tests__/ipc-handlers.test.ts index 207eb487..cd730050 100644 --- a/apps/frontend/src/main/__tests__/ipc-handlers.test.ts +++ b/apps/frontend/src/main/__tests__/ipc-handlers.test.ts @@ -153,8 +153,9 @@ function cleanupTestDirs(): void { } } -// Increase timeout for all tests in this file due to dynamic imports and setup overhead -describe("IPC Handlers", { timeout: 15000 }, () => { +// Increase timeout for all tests in this file due to dynamic imports and setup overhead. +// Windows requires longer timeout due to slower file system operations and module loading. +describe("IPC Handlers", { timeout: 30000 }, () => { let ipcMain: EventEmitter & { handlers: Map; invokeHandler: (channel: string, event: unknown, ...args: unknown[]) => Promise; diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts b/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts index d0602300..1247b6f2 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts @@ -4,6 +4,7 @@ const mockGetAPIProfileEnv = vi.fn(); const mockGetOAuthModeClearVars = vi.fn(); const mockGetPythonEnv = vi.fn(); const mockGetBestAvailableProfileEnv = vi.fn(); +const mockGetGitHubTokenForSubprocess = vi.fn(); vi.mock('../../../../services/profile', () => ({ getAPIProfileEnv: (...args: unknown[]) => mockGetAPIProfileEnv(...args), @@ -23,6 +24,12 @@ vi.mock('../../../../rate-limit-detector', () => ({ getBestAvailableProfileEnv: () => mockGetBestAvailableProfileEnv(), })); +// Mock getGitHubTokenForSubprocess to avoid calling gh CLI in tests +// Path is relative to the module being mocked (runner-env.ts), which imports from '../utils' +vi.mock('../../utils', () => ({ + getGitHubTokenForSubprocess: () => mockGetGitHubTokenForSubprocess(), +})); + import { getRunnerEnv } from '../runner-env'; describe('getRunnerEnv', () => { @@ -42,6 +49,8 @@ describe('getRunnerEnv', () => { profileName: 'Default', wasSwapped: false }); + // Default mock for GitHub token - returns null (no token) by default + mockGetGitHubTokenForSubprocess.mockResolvedValue(null); }); it('merges Python env with API profile env and OAuth clear vars', async () => { @@ -130,4 +139,24 @@ describe('getRunnerEnv', () => { // extraEnv has highest precedence expect(result.SHARED_VAR).toBe('from-extra'); }); + + it('includes GitHub token from gh CLI when available (fixes #151)', async () => { + mockGetAPIProfileEnv.mockResolvedValue({}); + mockGetOAuthModeClearVars.mockReturnValue({}); + mockGetGitHubTokenForSubprocess.mockResolvedValue('gh-token-123'); + + const result = await getRunnerEnv(); + + expect(result.GITHUB_TOKEN).toBe('gh-token-123'); + }); + + it('omits GITHUB_TOKEN when gh CLI returns null', async () => { + mockGetAPIProfileEnv.mockResolvedValue({}); + mockGetOAuthModeClearVars.mockReturnValue({}); + mockGetGitHubTokenForSubprocess.mockResolvedValue(null); + + const result = await getRunnerEnv(); + + expect(result.GITHUB_TOKEN).toBeUndefined(); + }); }); diff --git a/apps/frontend/src/renderer/components/Sidebar.tsx b/apps/frontend/src/renderer/components/Sidebar.tsx index 7a37cfa9..076c33d0 100644 --- a/apps/frontend/src/renderer/components/Sidebar.tsx +++ b/apps/frontend/src/renderer/components/Sidebar.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo } from 'react'; +import { useState, useEffect, useMemo, useRef } from 'react'; import { useTranslation } from 'react-i18next'; import { Plus, @@ -47,12 +47,17 @@ import { initializeProject } from '../stores/project-store'; import { useSettingsStore, saveSettings } from '../stores/settings-store'; +import { + useProjectEnvStore, + loadProjectEnvConfig, + clearProjectEnvConfig +} from '../stores/project-env-store'; import { AddProjectModal } from './AddProjectModal'; import { GitSetupModal } from './GitSetupModal'; import { RateLimitIndicator } from './RateLimitIndicator'; import { ClaudeCodeStatusBadge } from './ClaudeCodeStatusBadge'; import { UpdateBanner } from './UpdateBanner'; -import type { Project, AutoBuildVersionInfo, GitStatus, ProjectEnvConfig } from '../../shared/types'; +import type { Project, AutoBuildVersionInfo, GitStatus } from '../../shared/types'; export type SidebarView = 'kanban' | 'terminals' | 'roadmap' | 'context' | 'ideation' | 'github-issues' | 'gitlab-issues' | 'github-prs' | 'gitlab-merge-requests' | 'changelog' | 'insights' | 'worktrees' | 'agent-tools'; @@ -112,7 +117,6 @@ export function Sidebar({ const [gitStatus, setGitStatus] = useState(null); const [pendingProject, setPendingProject] = useState(null); const [isInitializing, setIsInitializing] = useState(false); - const [envConfig, setEnvConfig] = useState(null); const selectedProject = projects.find((p) => p.id === selectedProjectId); @@ -123,41 +127,55 @@ export function Sidebar({ saveSettings({ sidebarCollapsed: !isCollapsed }); }; - // Load env config when project changes to check GitHub/GitLab enabled state - useEffect(() => { - const loadEnvConfig = async () => { - if (selectedProject?.autoBuildPath) { - try { - const result = await window.electronAPI.getProjectEnv(selectedProject.id); - if (result.success && result.data) { - setEnvConfig(result.data); - } else { - setEnvConfig(null); - } - } catch { - setEnvConfig(null); - } - } else { - setEnvConfig(null); - } - }; - loadEnvConfig(); - }, [selectedProject?.id, selectedProject?.autoBuildPath]); + // Subscribe to project-env-store for reactive GitHub/GitLab tab visibility + const githubEnabled = useProjectEnvStore((state) => state.envConfig?.githubEnabled ?? false); + const gitlabEnabled = useProjectEnvStore((state) => state.envConfig?.gitlabEnabled ?? false); - // Compute visible nav items based on GitHub/GitLab enabled state + // Track the last loaded project ID to avoid redundant loads + const lastLoadedProjectIdRef = useRef(null); + + // Compute visible nav items based on GitHub/GitLab enabled state from store const visibleNavItems = useMemo(() => { const items = [...baseNavItems]; - if (envConfig?.githubEnabled) { + if (githubEnabled) { items.push(...githubNavItems); } - if (envConfig?.gitlabEnabled) { + if (gitlabEnabled) { items.push(...gitlabNavItems); } return items; - }, [envConfig?.githubEnabled, envConfig?.gitlabEnabled]); + }, [githubEnabled, gitlabEnabled]); + + // Load envConfig when project changes to ensure store is populated + useEffect(() => { + // Track whether this effect is still current (for race condition handling) + let isCurrent = true; + + const initializeEnvConfig = async () => { + if (selectedProject?.id && selectedProject?.autoBuildPath) { + // Only reload if the project ID differs from what we last loaded + if (selectedProject.id !== lastLoadedProjectIdRef.current) { + lastLoadedProjectIdRef.current = selectedProject.id; + await loadProjectEnvConfig(selectedProject.id); + // Check if this effect was cancelled while loading + if (!isCurrent) return; + } + } else { + // Clear the store if no project is selected or has no autoBuildPath + lastLoadedProjectIdRef.current = null; + clearProjectEnvConfig(); + } + }; + initializeEnvConfig(); + + // Cleanup function to mark this effect as stale + return () => { + isCurrent = false; + }; + }, [selectedProject?.id, selectedProject?.autoBuildPath]); // Keyboard shortcuts useEffect(() => { diff --git a/apps/frontend/src/renderer/components/project-settings/IntegrationSettings.tsx b/apps/frontend/src/renderer/components/project-settings/IntegrationSettings.tsx index 5e3bb375..332c77eb 100644 --- a/apps/frontend/src/renderer/components/project-settings/IntegrationSettings.tsx +++ b/apps/frontend/src/renderer/components/project-settings/IntegrationSettings.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; import { Zap, Eye, @@ -79,25 +79,39 @@ export function IntegrationSettings({ const [branches, setBranches] = useState([]); const [isLoadingBranches, setIsLoadingBranches] = useState(false); - // Load branches when GitHub section expands - useEffect(() => { - if (githubExpanded && project.path) { - loadBranches(); - } - // eslint-disable-next-line react-hooks/exhaustive-deps -- loadBranches is intentionally excluded to avoid infinite loops - }, [githubExpanded, project.path]); + // Track whether initial branch detection has been done to prevent double-execution + const hasDetectedMainBranch = useRef(false); + // Track mainBranch in a ref to avoid stale closure issues in loadBranches callback + const mainBranchRef = useRef(settings.mainBranch); - const loadBranches = async () => { + // Keep mainBranchRef in sync with settings.mainBranch + useEffect(() => { + mainBranchRef.current = settings.mainBranch; + }, [settings.mainBranch]); + + // Reset detection flag when project OR GitHub repo changes + // This allows auto-detection to run for a new repo within the same project + useEffect(() => { + hasDetectedMainBranch.current = false; + }, [project.path, envConfig?.githubRepo]); + + // Load branches function wrapped in useCallback + // Note: We use refs for mainBranch check and detection tracking to avoid stale closures + const loadBranches = useCallback(async () => { setIsLoadingBranches(true); try { const result = await window.electronAPI.getGitBranches(project.path); if (result.success && result.data) { setBranches(result.data); - // Auto-detect main branch if not set - if (!settings.mainBranch) { + // Auto-detect main branch if not set and not already detected + // Use mainBranchRef to avoid stale closure issues + if (!mainBranchRef.current && !hasDetectedMainBranch.current) { + hasDetectedMainBranch.current = true; const detectResult = await window.electronAPI.detectMainBranch(project.path); - if (detectResult.success && detectResult.data) { - setSettings(prev => ({ ...prev, mainBranch: detectResult.data! })); + // Re-check mainBranchRef after await - user may have selected a branch during detection + if (detectResult.success && detectResult.data !== null && detectResult.data !== undefined && !mainBranchRef.current) { + const detectedBranch = detectResult.data; + setSettings(prev => ({ ...prev, mainBranch: detectedBranch })); } } } @@ -106,7 +120,24 @@ export function IntegrationSettings({ } finally { setIsLoadingBranches(false); } - }; + // settings.mainBranch not in deps - we use mainBranchRef to avoid stale closures + // hasDetectedMainBranch ref tracks whether detection has run this session + }, [project.path, setSettings]); + + // Load branches when GitHub section expands or GitHub connection changes + useEffect(() => { + // Only load branches when: + // 1. GitHub section is expanded + // 2. Project path exists + // 3. GitHub is enabled with repo configured + if (!githubExpanded || !project.path) return; + if (!envConfig?.githubEnabled || !envConfig?.githubRepo) return; + + // Only load branches when we have a successful connection + if (gitHubConnectionStatus?.connected) { + loadBranches(); + } + }, [githubExpanded, project.path, envConfig?.githubEnabled, envConfig?.githubRepo, gitHubConnectionStatus?.connected, loadBranches]); if (!envConfig) return null; diff --git a/apps/frontend/src/renderer/components/project-settings/README.md b/apps/frontend/src/renderer/components/project-settings/README.md index d7b42c44..57e50827 100644 --- a/apps/frontend/src/renderer/components/project-settings/README.md +++ b/apps/frontend/src/renderer/components/project-settings/README.md @@ -226,11 +226,9 @@ hooks/ **Returns**: - `envConfig`: Current environment config - `setEnvConfig`: Config updater -- `updateEnvConfig`: Partial update function +- `updateEnvConfig`: Partial update function (auto-saves to backend) - `isLoadingEnv`: Loading state - `envError`: Error state -- `isSavingEnv`: Save in progress state -- `saveEnvConfig`: Save function ### useClaudeAuth.ts **Purpose**: Manages Claude authentication status checking. diff --git a/apps/frontend/src/renderer/components/project-settings/hooks/useProjectSettings.ts b/apps/frontend/src/renderer/components/project-settings/hooks/useProjectSettings.ts index a0307119..7dc7f286 100644 --- a/apps/frontend/src/renderer/components/project-settings/hooks/useProjectSettings.ts +++ b/apps/frontend/src/renderer/components/project-settings/hooks/useProjectSettings.ts @@ -1,10 +1,11 @@ -import { useState, useEffect } from 'react'; +import { useState, useEffect, useRef } from 'react'; import { updateProjectSettings, checkProjectVersion, initializeProject } from '../../../stores/project-store'; import { checkGitHubConnection as checkGitHubConnectionGlobal } from '../../../stores/github'; +import { setProjectEnvConfig } from '../../../stores/project-env-store'; import type { Project, ProjectSettings as ProjectSettingsType, @@ -34,7 +35,6 @@ export interface UseProjectSettingsReturn { isLoadingEnv: boolean; envError: string | null; setEnvError: React.Dispatch>; - isSavingEnv: boolean; updateEnvConfig: (updates: Partial) => Promise; // Password visibility toggles @@ -74,7 +74,6 @@ export interface UseProjectSettingsReturn { // Actions handleInitialize: () => Promise; - handleSaveEnv: () => Promise; handleClaudeSetup: () => Promise; handleSave: (onClose: () => void) => Promise; } @@ -91,10 +90,17 @@ export function useProjectSettings( const [isUpdating, setIsUpdating] = useState(false); // Environment configuration state + // NOTE: We maintain local envConfig state AND update the global project-env-store. + // This dual-state pattern is intentional: + // - Local state: allows dialog-scoped edits, immediate UI feedback + // - Global store: syncs to Sidebar and other components for real-time updates + // The local state is the source of truth for this dialog's edits. const [envConfig, setEnvConfig] = useState(null); const [isLoadingEnv, setIsLoadingEnv] = useState(false); const [envError, setEnvError] = useState(null); - const [isSavingEnv, setIsSavingEnv] = useState(false); + // Ref to track the latest committed config - used to handle concurrent updateEnvConfig calls + // This ensures rapid updates don't lose changes due to stale state reads + const committedEnvConfigRef = useRef(null); // Password visibility toggles const [showClaudeToken, setShowClaudeToken] = useState(false); @@ -156,6 +162,9 @@ export function useProjectSettings( const result = await window.electronAPI.getProjectEnv(project.id); if (result.success && result.data) { setEnvConfig(result.data); + committedEnvConfigRef.current = result.data; + // Update the shared store so other components (like Sidebar) can react + setProjectEnvConfig(project.id, result.data); } else { setEnvError(result.error || 'Failed to load environment config'); } @@ -287,6 +296,9 @@ export function useProjectSettings( const envResult = await window.electronAPI.getProjectEnv(project.id); if (envResult.success && envResult.data) { setEnvConfig(envResult.data); + committedEnvConfigRef.current = envResult.data; + // Update global store so Sidebar reflects correct GitHub/GitLab enabled state + setProjectEnvConfig(project.id, envResult.data); } } else { setError(result?.error || 'Failed to initialize'); @@ -298,23 +310,6 @@ export function useProjectSettings( } }; - const handleSaveEnv = async () => { - if (!envConfig) return; - - setIsSavingEnv(true); - setEnvError(null); - try { - const result = await window.electronAPI.updateProjectEnv(project.id, envConfig); - if (!result.success) { - setEnvError(result.error || 'Failed to save environment config'); - } - } catch (err) { - setEnvError(err instanceof Error ? err.message : 'Unknown error'); - } finally { - setIsSavingEnv(false); - } - }; - const handleClaudeSetup = async () => { setIsCheckingClaudeAuth(true); try { @@ -324,6 +319,9 @@ export function useProjectSettings( const envResult = await window.electronAPI.getProjectEnv(project.id); if (envResult.success && envResult.data) { setEnvConfig(envResult.data); + committedEnvConfigRef.current = envResult.data; + // Update global store so Sidebar and other components reflect changes + setProjectEnvConfig(project.id, envResult.data); } } } catch { @@ -361,22 +359,42 @@ export function useProjectSettings( }; const updateEnvConfig = async (updates: Partial) => { - if (envConfig) { - const newConfig = { ...envConfig, ...updates }; + // Use the committed ref as base to handle concurrent calls correctly + // This ensures rapid updates don't lose changes due to stale state reads + const baseConfig = committedEnvConfigRef.current || envConfig; + if (!baseConfig) return; - // Save to backend FIRST so disk is updated before effects run - try { - const result = await window.electronAPI.updateProjectEnv(project.id, newConfig); - if (!result.success) { - console.error('[useProjectSettings] Failed to auto-save env config:', result.error); - } - } catch (err) { - console.error('[useProjectSettings] Error auto-saving env config:', err); + const newConfig = { ...baseConfig, ...updates }; + + // Update the ref BEFORE the await (optimistically) to prevent race conditions + // If two calls happen rapidly, the second will see the first's changes in the ref + committedEnvConfigRef.current = newConfig; + + // Save to backend + try { + const result = await window.electronAPI.updateProjectEnv(project.id, newConfig); + if (!result.success) { + console.error('[useProjectSettings] Failed to auto-save env config:', result.error); + setEnvError(result.error || 'Failed to save environment config'); + // Note: We don't rollback the ref here because another concurrent call may have + // already updated it. The error is shown to the user who can retry. + return; } - - // Then update local state (triggers effects that read from disk) - setEnvConfig(newConfig); + } catch (err) { + console.error('[useProjectSettings] Error auto-saving env config:', err); + setEnvError(err instanceof Error ? err.message : 'Failed to save environment config'); + // Note: We don't rollback the ref here for the same reason as above. + return; } + + // Clear any previous error on successful save + setEnvError(null); + + // Update local state after successful backend save + setEnvConfig(newConfig); + + // Update the shared store so other components (like Sidebar) can react immediately + setProjectEnvConfig(project.id, newConfig); }; return { @@ -393,7 +411,6 @@ export function useProjectSettings( isLoadingEnv, envError, setEnvError, - isSavingEnv, updateEnvConfig, showClaudeToken, setShowClaudeToken, @@ -419,7 +436,6 @@ export function useProjectSettings( linearConnectionStatus, isCheckingLinear, handleInitialize, - handleSaveEnv, handleClaudeSetup, handleSave }; diff --git a/apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts b/apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts index c1d53615..bcdef425 100644 --- a/apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts +++ b/apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts @@ -25,7 +25,6 @@ export function createHookProxy( get isLoadingEnv() { return hookRef.current.isLoadingEnv; }, get envError() { return hookRef.current.envError; }, get setEnvError() { return hookRef.current.setEnvError; }, - get isSavingEnv() { return hookRef.current.isSavingEnv; }, get updateEnvConfig() { return hookRef.current.updateEnvConfig; }, get showClaudeToken() { return hookRef.current.showClaudeToken; }, get setShowClaudeToken() { return hookRef.current.setShowClaudeToken; }, @@ -51,7 +50,6 @@ export function createHookProxy( get linearConnectionStatus() { return hookRef.current.linearConnectionStatus; }, get isCheckingLinear() { return hookRef.current.isCheckingLinear; }, get handleInitialize() { return hookRef.current.handleInitialize; }, - get handleSaveEnv() { return hookRef.current.handleSaveEnv; }, get handleClaudeSetup() { return hookRef.current.handleClaudeSetup; }, get handleSave() { return hookRef.current.handleSave; }, }; diff --git a/apps/frontend/src/renderer/stores/project-env-store.ts b/apps/frontend/src/renderer/stores/project-env-store.ts new file mode 100644 index 00000000..c04a5cf1 --- /dev/null +++ b/apps/frontend/src/renderer/stores/project-env-store.ts @@ -0,0 +1,137 @@ +import { create } from 'zustand'; +import type { ProjectEnvConfig } from '../../shared/types'; + +interface ProjectEnvState { + // State + envConfig: ProjectEnvConfig | null; + projectId: string | null; + isLoading: boolean; + error: string | null; + // Track the current pending request to handle race conditions + // Stored in state so it's properly reset on HMR and managed alongside other state + currentRequestId: number; + + // Actions + setEnvConfig: (projectId: string | null, config: ProjectEnvConfig | null) => void; + setEnvConfigOnly: (projectId: string | null, config: ProjectEnvConfig | null) => void; + clearEnvConfig: () => void; + setLoading: (loading: boolean) => void; + setError: (error: string | null) => void; + incrementRequestId: () => number; +} + +export const useProjectEnvStore = create((set, get) => ({ + // Initial state + envConfig: null, + projectId: null, + isLoading: false, + error: null, + currentRequestId: 0, + + // Actions + // setEnvConfig clears error - used for successful config loads + setEnvConfig: (projectId, envConfig) => set({ + projectId, + envConfig, + error: null + }), + + // setEnvConfigOnly updates config without touching error state - used in error cases + setEnvConfigOnly: (projectId, envConfig) => set({ + projectId, + envConfig + }), + + clearEnvConfig: () => set({ + envConfig: null, + projectId: null, + error: null + }), + + setLoading: (isLoading) => set({ isLoading }), + + setError: (error) => set({ error }), + + incrementRequestId: () => { + const newId = get().currentRequestId + 1; + set({ currentRequestId: newId }); + return newId; + } +})); + +/** + * Load project environment config from main process. + * Updates the store with the loaded config. + * Handles race conditions when called rapidly for different projects. + */ +export async function loadProjectEnvConfig(projectId: string): Promise { + // Get fresh store state for initial operations + const initialStore = useProjectEnvStore.getState(); + + // Increment request ID to track this specific request + const requestId = initialStore.incrementRequestId(); + + initialStore.setLoading(true); + initialStore.setError(null); + + try { + const result = await window.electronAPI.getProjectEnv(projectId); + + // Get fresh store state after async operation for consistency + const currentStore = useProjectEnvStore.getState(); + + // Check if this request is still the current one (handle race conditions) + if (requestId !== currentStore.currentRequestId) { + // A newer request was made, ignore this result + return null; + } + + if (result.success && result.data) { + currentStore.setEnvConfig(projectId, result.data); + return result.data; + } else { + // Use setEnvConfigOnly to update config without clearing the error we're about to set + currentStore.setEnvConfigOnly(projectId, null); + currentStore.setError(result.error || 'Failed to load environment config'); + return null; + } + } catch (error) { + // Get fresh store state after async operation + const currentStore = useProjectEnvStore.getState(); + + // Check if this request is still the current one + if (requestId !== currentStore.currentRequestId) { + return null; + } + + // Use setEnvConfigOnly to update config without clearing the error we're about to set + currentStore.setEnvConfigOnly(projectId, null); + currentStore.setError(error instanceof Error ? error.message : 'Unknown error'); + return null; + } finally { + // Get fresh store state for final loading state update + const finalStore = useProjectEnvStore.getState(); + // Only update loading state if this is still the current request + if (requestId === finalStore.currentRequestId) { + finalStore.setLoading(false); + } + } +} + +/** + * Set project env config directly (for use by useProjectSettings hook). + * This is a standalone function for use outside React components. + */ +export function setProjectEnvConfig(projectId: string, config: ProjectEnvConfig | null): void { + const store = useProjectEnvStore.getState(); + store.setEnvConfig(projectId, config); +} + +/** + * Clear the project env config (for use when switching projects or closing dialogs). + * This is a standalone function for use outside React components. + */ +export function clearProjectEnvConfig(): void { + const store = useProjectEnvStore.getState(); + store.clearEnvConfig(); +}