Conversation
When users authenticate with Claude during the GitHub setup modal, the OAuth token is now automatically saved to the active Claude profile instead of being ignored. Changes: - Modified handleOAuthToken() to save tokens to active profile when not in a profile-specific terminal (e.g., during GitHub OAuth flow) - Added Claude authentication step to GitHub setup modal flow - Renamed setup steps for clarity: 'auth' → 'github-auth',
Added defensive null check when saving OAuth tokens to active profile during non-profile terminal flows (e.g., GitHub setup). Previously, if no active profile existed, the code would crash when trying to access activeProfile.id. Changes: - Added null check for activeProfile before attempting to save token - Send failure event to UI when no active profile is found - Include profileId in failure event for consistency - Added docstrings to fall
- Fix HTTP 415 error when downloading updates from GitHub API - Use 'application/vnd.github+json' Accept header for API URLs - Use 'application/octet-stream' only for CDN/direct download URLs - Add getEffectiveVersion() to track installed source version - Reads from .update-metadata.json written during updates - Works in both dev mode and packaged app - Falls back to app.getVersion() if no metadata found - Update version display to persist after app reload - APP_VERSION IPC now returns effective version - Update checker uses effective version for comparison - UI updates displayVersion from check result - Add comprehensive DEBUG logging for update process - Logs update stages: download, extract, apply - Logs version resolution and metadata paths - Shows clear success/failure banners 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When switching Claude profiles via the UI, existing terminal sessions now automatically restart with the new profile's OAuth token. This fixes the issue where users had to manually restart Claude after switching profiles. Changes: - Profile switch handler now iterates active terminals and restarts Claude sessions that are in Claude mode - Added clear terminal before profile switch to hide temp file command - Fixed OAuth token regex to match 'default' profile ID (not just profile-\d+) - Hide "Authenticate" button when profile is already authenticated - Add re-authenticate button (refresh icon) for authenticated profiles - Added debug logging for profile switching (enabled with DEBUG=true) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@AndyMik90 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughExports a new getEffectiveVersion(), switches version lookups to it across updater and IPC; adds AutoBuild update progress field newVersion; adds extensive debug logging, native drag-and-drop, Git branch selection UI, Claude OAuth flow and async per-terminal Claude profile switching with Promise.allSettled. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auto-claude-ui/src/main/ipc-handlers/autobuild-source-handlers.ts (1)
53-99: Race condition: Initial progress message sent after download starts.The "checking" progress message (lines 92-99) is sent after
downloadAndApplyUpdateis invoked (line 54). If the download progresses quickly, the UI could receive download progress events before the initial "checking" state, causing confusing state transitions.🔎 Proposed fix: Send initial progress before starting download
ipcMain.on( IPC_CHANNELS.AUTOBUILD_SOURCE_DOWNLOAD, () => { debugLog('[IPC] Autobuild source download requested'); const mainWindow = getMainWindow(); if (!mainWindow) { debugLog('[IPC] No main window available, aborting update'); return; } + // Send initial progress before starting download + mainWindow.webContents.send( + IPC_CHANNELS.AUTOBUILD_SOURCE_PROGRESS, + { + stage: 'checking', + message: 'Starting update...' + } as AutoBuildSourceUpdateProgress + ); + // Start download in background downloadAndApplyUpdate((progress) => { debugLog('[IPC] Update progress:', progress.stage, progress.message); mainWindow.webContents.send( IPC_CHANNELS.AUTOBUILD_SOURCE_PROGRESS, progress ); }).then((result) => { // ... rest of handler }); - - // Send initial progress - mainWindow.webContents.send( - IPC_CHANNELS.AUTOBUILD_SOURCE_PROGRESS, - { - stage: 'checking', - message: 'Starting update...' - } as AutoBuildSourceUpdateProgress - ); } );
🧹 Nitpick comments (6)
auto-claude-ui/src/renderer/components/settings/IntegrationSettings.tsx (1)
407-440: Consider extracting the authentication check to reduce duplication.The authentication logic
profile.oauthToken || (profile.isDefault && profile.configDir)is duplicated from the badge display logic at line 387. Extracting this to a derived variable would improve maintainability and ensure consistency if the logic changes.🔎 Proposed refactor to extract authentication check
Within the
claudeProfiles.mapcallback (around line 321), add a derived variable:{claudeProfiles.map((profile) => { + const isAuthenticated = profile.oauthToken || (profile.isDefault && profile.configDir); return ( <div key={profile.id}Then use it in both places:
- {(profile.oauthToken || (profile.isDefault && profile.configDir)) ? ( + {isAuthenticated ? ( <span className="text-xs bg-success/20 text-success px-1.5 py-0.5 rounded flex items-center gap-1">- {!(profile.oauthToken || (profile.isDefault && profile.configDir)) ? ( + {!isAuthenticated ? ( <Buttonauto-claude-ui/src/renderer/components/settings/AdvancedSettings.tsx (1)
178-191: Consider using a consistent logging utility.The
console.logstatements here differ from thedebugLogutility used in the main process for debug logging. For consistency and to allow conditional debug output in production, consider using a renderer-compatible debug logger or removing these before release.🔎 Suggested approach
Either remove the console.log statements before release, or create/use a shared debug logging utility that can be conditionally enabled:
- console.log('[AdvancedSettings] Checking for source updates...'); + // Remove or use a debug utility if needed for troubleshootingauto-claude-ui/src/main/updater/version-manager.ts (1)
60-74: Consider validating the parsed metadata structure.The type assertion
as UpdateMetadataassumes the JSON structure matches without runtime validation. If the file contains unexpected content,metadata.versioncould be a non-string value that passes the truthy check but causes issues downstream.🔎 Proposed defensive validation
try { const metadata = JSON.parse(readFileSync(metadataPath, 'utf-8')) as UpdateMetadata; - if (metadata.version) { + if (typeof metadata.version === 'string' && metadata.version) { if (isDebug) { console.log(`[Version] Found metadata version: ${metadata.version}`); } return metadata.version; } } catch (e) {auto-claude-ui/src/main/ipc-handlers/autobuild-source-handlers.ts (1)
25-34: Redundant logging: bothconsole.loganddebugLogfor the same events.Using both
console.loganddebugLogcreates duplicate output. ThedebugLogutility is designed for conditional debug output, whileconsole.logalways emits. Consider using onlydebugLogfor debug-level messages, or onlyconsole.logif you want unconditional logging.🔎 Proposed simplification using debugLog only
ipcMain.handle( IPC_CHANNELS.AUTOBUILD_SOURCE_CHECK, async (): Promise<IPCResult<{ updateAvailable: boolean; currentVersion: string; latestVersion?: string; releaseNotes?: string; releaseUrl?: string; error?: string }>> => { - console.log('[autobuild-source] Check for updates called'); debugLog('[IPC] AUTOBUILD_SOURCE_CHECK called'); try { const result = await checkSourceUpdates(); - console.log('[autobuild-source] Check result:', JSON.stringify(result, null, 2)); debugLog('[IPC] AUTOBUILD_SOURCE_CHECK result:', result); return { success: true, data: result }; } catch (error) { - console.error('[autobuild-source] Check error:', error); debugLog('[IPC] AUTOBUILD_SOURCE_CHECK error:', error);auto-claude-ui/src/main/ipc-handlers/terminal-handlers.ts (1)
284-290: Consider logging exceptions unconditionally for production observability.
debugErroronly logs when debug mode is enabled. Exceptions in this handler may go unnoticed in production.🔎 Suggested change
} catch (error) { - debugError('[terminal-handlers:CLAUDE_PROFILE_SET_ACTIVE] EXCEPTION:', error); + console.error('[terminal-handlers:CLAUDE_PROFILE_SET_ACTIVE] EXCEPTION:', error); return { success: false, error: error instanceof Error ? error.message : 'Failed to set active Claude profile' }; }auto-claude-ui/src/main/terminal/claude-integration-handler.ts (1)
166-260: Consider consolidating logging patterns for maintainability.The code uses extensive dual logging (
console.warn+debugLog) throughoutinvokeClaudeandswitchClaudeProfile. While the "always-on tracing" (line 299) appears intentional for production troubleshooting, this results in:
- Very verbose production logs that could impact log storage and analysis
- Some redundancy between always-on and debug logs
- Potential performance impact when debug is enabled
Consider:
- Reduce production logging: Keep only critical state transitions in
console.warn- Structured logging: Use a logging library with levels (INFO, DEBUG, TRACE) instead of mixing console.warn/debugLog
- Log aggregation: Consolidate related data into single log statements rather than multiple sequential logs
- Sampling: For high-frequency operations, consider log sampling in production
Example: More structured logging approach
// Use a structured logger with levels const logger = { info: (...args: unknown[]) => console.log(...args), warn: (...args: unknown[]) => console.warn(...args), debug: (...args: unknown[]) => debugLog(...args), error: (...args: unknown[]) => console.error(...args) }; // Then consolidate logs: logger.debug('[ClaudeIntegration:invokeClaude] Starting invocation', { terminalId: terminal.id, profileId, cwd, profileResolution: { previousProfileId, newProfileId: activeProfile?.id, profileName: activeProfile?.name, hasOAuthToken: !!activeProfile?.oauthToken } });Also applies to: 299-366
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
auto-claude-ui/src/main/auto-claude-updater.ts(1 hunks)auto-claude-ui/src/main/ipc-handlers/autobuild-source-handlers.ts(5 hunks)auto-claude-ui/src/main/ipc-handlers/settings-handlers.ts(2 hunks)auto-claude-ui/src/main/ipc-handlers/terminal-handlers.ts(2 hunks)auto-claude-ui/src/main/terminal/claude-integration-handler.ts(6 hunks)auto-claude-ui/src/main/updater/http-client.ts(1 hunks)auto-claude-ui/src/main/updater/update-checker.ts(4 hunks)auto-claude-ui/src/main/updater/update-installer.ts(8 hunks)auto-claude-ui/src/main/updater/version-manager.ts(1 hunks)auto-claude-ui/src/renderer/components/TaskCreationWizard.tsx(2 hunks)auto-claude-ui/src/renderer/components/settings/AdvancedSettings.tsx(5 hunks)auto-claude-ui/src/renderer/components/settings/IntegrationSettings.tsx(2 hunks)auto-claude-ui/src/shared/types/settings.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T15:01:23.017Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T15:01:23.017Z
Learning: Set up OAuth token by running `claude setup-token` and adding `CLAUDE_CODE_OAUTH_TOKEN=your-token` to `auto-claude/.env`
Applied to files:
auto-claude-ui/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (7)
auto-claude-ui/src/main/ipc-handlers/settings-handlers.ts (1)
auto-claude-ui/src/main/updater/version-manager.ts (1)
getEffectiveVersion(25-83)
auto-claude-ui/src/main/ipc-handlers/terminal-handlers.ts (2)
auto-claude-ui/src/shared/utils/debug-logger.ts (2)
debugLog(13-17)debugError(25-29)auto-claude-ui/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(531-536)
auto-claude-ui/src/main/updater/version-manager.ts (2)
auto-claude-ui/src/main/auto-claude-updater.ts (3)
getBundledVersion(30-30)getEffectiveVersion(30-30)UpdateMetadata(26-26)auto-claude-ui/src/__mocks__/electron.ts (1)
app(8-22)
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (2)
auto-claude-ui/src/shared/utils/debug-logger.ts (2)
debugLog(13-17)debugError(25-29)auto-claude-ui/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(531-536)
auto-claude-ui/src/main/updater/update-installer.ts (5)
auto-claude-ui/src/shared/utils/debug-logger.ts (1)
debugLog(13-17)auto-claude-ui/src/main/updater/update-checker.ts (2)
getCachedRelease(17-19)setCachedRelease(24-26)auto-claude-ui/src/main/updater/http-client.ts (1)
fetchJson(12-64)auto-claude-ui/src/main/updater/version-manager.ts (1)
parseVersionFromTag(89-92)auto-claude-ui/src/main/updater/file-operations.ts (2)
extractTarball(17-57)copyDirectoryRecursive(62-96)
auto-claude-ui/src/renderer/components/settings/AdvancedSettings.tsx (1)
auto-claude-ui/scripts/download-prebuilds.js (1)
version(32-32)
auto-claude-ui/src/main/updater/update-checker.ts (2)
auto-claude-ui/src/main/updater/version-manager.ts (2)
getEffectiveVersion(25-83)compareVersions(98-111)auto-claude-ui/src/shared/utils/debug-logger.ts (1)
debugLog(13-17)
🔇 Additional comments (25)
auto-claude-ui/src/renderer/components/settings/IntegrationSettings.tsx (1)
426-439: LGTM! The re-authenticate button implementation is correct.The ghost variant button with
RefreshCwicon provides a subtle but accessible re-authentication option for already authenticated profiles. The loading state handling and disabled logic are properly implemented.Minor accessibility suggestion: Consider adding
aria-label="Re-authenticate profile"alongside thetitleattribute for better screen reader support on icon-only buttons.auto-claude-ui/src/renderer/components/TaskCreationWizard.tsx (1)
6-6: LGTM! Good choice of collision detection strategy.Using
pointerWithinis appropriate here since you have nested drop zones (the description textarea inside the form). This ensures the innermost drop zone under the pointer receives the drop, which is the expected behavior for file @mentions vs. adding to referenced files list.Also applies to: 639-639
auto-claude-ui/src/main/updater/http-client.ts (2)
77-83: LGTM! Dynamic Accept header handling is correct.The conditional logic properly handles the GitHub API redirect flow: API requests need
application/vnd.github+jsonto get the redirect, while the subsequent CDN download needsapplication/octet-streamfor binary content.
87-94: Good resource cleanup before redirect.Properly closing the file stream before the recursive call prevents file handle leaks. The redirect URL will receive the correct
application/octet-streamheader since it won't containapi.github.com.auto-claude-ui/src/renderer/components/settings/AdvancedSettings.tsx (2)
79-92: LGTM! Version state management approach is sound.The local
displayVersionstate pattern allows the UI to immediately reflect version changes after updates without waiting for the parent component to receive and propagate the new version prop. The sync effect ensures the prop remains the source of truth when it changes externally.
108-111: Good handling of newVersion on update completion.This correctly updates the displayed version immediately when the update completes with a
newVersionvalue, providing instant feedback to the user.auto-claude-ui/src/main/updater/update-installer.ts (3)
29-30: LGTM! Comprehensive debug logging for update workflow.The debug logging provides excellent traceability through the update process. Using the shared
debugLogutility ensures logs are conditional and won't pollute production output unless debugging is enabled.Also applies to: 41-41, 48-53
157-172: Good visual separation for update result logging.The boxed summary blocks for success/failure make it easy to identify update outcomes in logs. This will be helpful for debugging update issues.
167-177: LGTM! Error handling improvement.Extracting
errorMessageonce and reusing it for both logging and the progress callback is cleaner than the previous approach. The error message is now consistently formatted.auto-claude-ui/src/main/ipc-handlers/settings-handlers.ts (1)
267-272: LGTM! Using effective version for APP_VERSION.Switching to
getEffectiveVersion()ensures the reported version reflects source updates, not just the bundled Electron app version. This aligns with the overall versioning strategy in this PR.Minor: Consider using
debugLoginstead ofconsole.logfor consistency with other logging in the updater modules, or remove the log statement before release.auto-claude-ui/src/main/auto-claude-updater.ts (1)
30-30: LGTM! Public API surface expanded appropriately.Exporting
getEffectiveVersionalongsidegetBundledVersionprovides consumers with the flexibility to get either the packaged app version or the effective version accounting for source updates. This is the right abstraction for the versioning strategy.auto-claude-ui/src/shared/types/settings.ts (1)
104-105: LGTM! Type extension for version propagation.The optional
newVersionfield enables the update progress payload to carry version information back to the UI, allowing immediate display update without additional IPC calls. Good documentation with the JSDoc comment.auto-claude-ui/src/main/updater/update-checker.ts (3)
38-41: LGTM! Using effective version for update checks.Switching to
getEffectiveVersion()ensures update checks compare against the currently installed source version, not just the bundled version. This prevents false "update available" notifications after a successful source update.
53-57: Good debug logging for update decision.These debug logs will be helpful for diagnosing version comparison issues. The logging is appropriately conditional via
debugLog.
66-75: Good error handling with cache cleanup.Clearing the cached release on error prevents stale data from being used in subsequent operations. Including
currentVersionin the error response allows the UI to still display the current version even when the update check fails.auto-claude-ui/src/main/updater/version-manager.ts (2)
5-17: LGTM!The imports are appropriate for the new functionality, and the updated documentation for
getBundledVersioncorrectly clarifies its role as the base version source.
25-83: Good fallback design with comprehensive path checking.The function handles both packaged and development environments well, with proper error handling that gracefully falls back to the bundled version. The debug logging will be useful for troubleshooting version resolution issues.
auto-claude-ui/src/main/ipc-handlers/autobuild-source-handlers.ts (4)
8-9: LGTM!The imports correctly bring in
getEffectiveVersionfor the updated version retrieval logic anddebugLogfor consistent logging.
48-51: Good defensive check for missing window.This guard prevents potential crashes when attempting to send IPC messages to a null window reference.
66-69: Good addition ofnewVersionto the progress payload.Including the version in the completion message enables the UI to refresh and display the updated version without a separate fetch.
103-118: LGTM!Correctly switched to
getEffectiveVersion()to report the actual running version after updates, rather than just the bundled version.auto-claude-ui/src/main/ipc-handlers/terminal-handlers.ts (2)
10-10: LGTM!The import aligns with the
debugLoganddebugErrorutilities defined in the shared debug-logger module.
212-279: Well-structured terminal profile switching logic.The approach is sound:
- Correctly detects profile changes before triggering switches
- Properly partitions terminals by Claude mode
- Uses
Promise.allSettledto avoid failing the main operation if individual switches fail- Provides detailed summary logging for debugging
The issues with excessive logging and error swallowing are flagged separately.
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (2)
13-13: LGTM - Debug logging utilities imported correctly.The debug utilities are properly imported and used consistently throughout the file for conditional logging.
96-97: LGTM - Default profile support added to OAuth token matching.The regex now correctly matches both custom profiles (
profile-\d+) and the default profile, enabling OAuth token auto-save for all profile types.
- Add collapsible Git Options section with base branch selector - Allow per-task override of the worktree base branch - Fetch and display available branches from the project repository - Show project default branch in placeholder when available - Use special placeholder value for Radix UI Select compatibility - Move DndContext inside DialogContent for proper portal behavior - Add drag-and-drop debugging logs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Implement default branch selection in GitHub integration settings - Fetch and display available branches based on the project path - Update TaskCreationWizard to support file reference drops in the description - Improve drag-and-drop handling for file references and images - Add console logging for agent process when DEBUG is enabled - Refactor FileTreeItem to manage drag state and custom drag images This update enhances user experience with Git operations and improves the task creation workflow by allowing users to easily reference files.
…itHub setup flow
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
auto-claude-ui/src/main/agent/agent-process.ts (1)
240-243: Consider extracting the DEBUG check to reduce duplication.The same DEBUG environment variable check appears in both the stdout and stderr handlers. Extracting this logic to a helper method or constant would improve maintainability and ensure consistency if the acceptable DEBUG values change in the future.
🔎 Proposed refactor to eliminate duplication
Add a helper method near the top of the class:
constructor(state: AgentState, events: AgentEvents, emitter: EventEmitter) { this.state = state; this.events = events; this.emitter = emitter; } + + /** + * Check if debug logging is enabled via DEBUG environment variable + */ + private isDebugEnabled(): boolean { + return ['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? ''); + }Then simplify both handlers:
childProcess.stdout?.on('data', (data: Buffer) => { const log = data.toString('utf8'); this.emitter.emit('log', taskId, log); processLog(log); // Print to console when DEBUG is enabled (visible in pnpm dev terminal) - if (['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? '')) { + if (this.isDebugEnabled()) { console.log(`[Agent:${taskId}] ${log.trim()}`); } });childProcess.stderr?.on('data', (data: Buffer) => { const log = data.toString('utf8'); this.emitter.emit('log', taskId, log); processLog(log); // Print to console when DEBUG is enabled (visible in pnpm dev terminal) - if (['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? '')) { + if (this.isDebugEnabled()) { console.log(`[Agent:${taskId}] ${log.trim()}`); } });Also applies to: 253-256
auto-claude-ui/src/renderer/components/FileTreeItem.tsx (1)
89-117: Consider adding error handling for drag image cleanup.The drag image cleanup logic works well, but could be more robust.
🔎 Optional: Add error handling for drag image cleanup
// Clean up drag image after a short delay setTimeout(() => { - document.body.removeChild(dragImage); + try { + if (dragImage.parentNode) { + document.body.removeChild(dragImage); + } + } catch (error) { + // Drag image already removed or component unmounted + } }, 0);This prevents potential errors if the element is already removed or the component unmounts before cleanup.
auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx (1)
84-85: Verify intentional omission offetchBranchesfrom dependencies.The eslint-disable comment suppresses the exhaustive-deps warning. This appears intentional since
fetchBranchesis defined within the component and would cause re-renders if included. However, consider usinguseCallbackforfetchBranchesto make the dependency relationship explicit and avoid the lint suppression.🔎 Alternative using useCallback
+ import { useState, useEffect, useCallback } from 'react'; ... - const fetchBranches = async () => { + const fetchBranches = useCallback(async () => { if (!projectPath) { debugLog('fetchBranches: No projectPath, skipping'); return; } // ... rest of function - }; + }, [projectPath, envConfig?.defaultBranch, updateEnvConfig]); useEffect(() => { if (envConfig?.githubEnabled && projectPath) { fetchBranches(); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [envConfig?.githubEnabled, projectPath]); + }, [envConfig?.githubEnabled, projectPath, fetchBranches]);auto-claude-ui/src/renderer/components/TaskCreationWizard.tsx (4)
172-179: Consider memoizing fetch functions or inlining the effect logic.The disabled
exhaustive-depslint rule suggests potential dependency management issues. While the current implementation may work, it's better to either:
- Wrap
fetchBranchesandfetchProjectDefaultBranchinuseCallbackand include them in the dependency array, or- Inline the fetch logic directly in the effect
This ensures the effect remains stable and reduces the risk of stale closures.
🔎 Proposed refactor using useCallback
Move the function definitions above the effect and wrap them:
+ const fetchBranches = useCallback(async () => { + if (!projectPath) return; + + setIsLoadingBranches(true); + try { + const result = await window.electronAPI.getGitBranches(projectPath); + if (result.success && result.data) { + setBranches(result.data); + } + } catch (err) { + console.error('Failed to fetch branches:', err); + } finally { + setIsLoadingBranches(false); + } + }, [projectPath]); + + const fetchProjectDefaultBranch = useCallback(async () => { + if (!projectId) return; + + try { + const result = await window.electronAPI.getProjectEnv(projectId); + if (result.success && result.data?.defaultBranch) { + setProjectDefaultBranch(result.data.defaultBranch); + } else if (projectPath) { + const detectResult = await window.electronAPI.detectMainBranch(projectPath); + if (detectResult.success && detectResult.data) { + setProjectDefaultBranch(detectResult.data); + } + } + } catch (err) { + console.error('Failed to fetch project default branch:', err); + } + }, [projectId, projectPath]); useEffect(() => { if (open && projectPath) { fetchBranches(); fetchProjectDefaultBranch(); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [open, projectPath]); + }, [open, projectPath, fetchBranches, fetchProjectDefaultBranch]);
181-195: Consider adding user-facing error feedback for branch fetching failures.Currently, branch fetch errors are only logged to the console. Users won't know if branch loading failed. Consider setting an error message or showing a toast notification when branch fetching fails.
🔎 Example with error state
const fetchBranches = async () => { if (!projectPath) return; setIsLoadingBranches(true); try { const result = await window.electronAPI.getGitBranches(projectPath); if (result.success && result.data) { setBranches(result.data); + } else { + setError('Failed to load Git branches'); } } catch (err) { console.error('Failed to fetch branches:', err); + setError('Failed to load Git branches'); } finally { setIsLoadingBranches(false); } };
197-215: Same error feedback suggestion applies here.Similar to
fetchBranches, errors in fetching the project default branch are only logged to the console. Consider providing user feedback, though this is less critical since the UI shows "Use project default" as an option regardless.
952-1009: LGTM: Well-structured Git options UI.The collapsible Git options section follows the established pattern and provides clear user feedback. The dynamic placeholder showing the actual default branch name (
Use project default (main)) is helpful for users.One minor note: the placeholder logic on line 991 and 995 is duplicated. Consider extracting it to a constant if this becomes harder to maintain.
🔎 Optional: Extract placeholder text to reduce duplication
+ const defaultBranchLabel = useMemo( + () => `Use project default${projectDefaultBranch ? ` (${projectDefaultBranch})` : ''}`, + [projectDefaultBranch] + ); + // ... in the Git Options section: <SelectTrigger id="base-branch" className="h-9"> - <SelectValue placeholder={`Use project default${projectDefaultBranch ? ` (${projectDefaultBranch})` : ''}`} /> + <SelectValue placeholder={defaultBranchLabel} /> </SelectTrigger> <SelectContent> - <SelectItem value={PROJECT_DEFAULT_BRANCH}> - Use project default{projectDefaultBranch ? ` (${projectDefaultBranch})` : ''} - </SelectItem> + <SelectItem value={PROJECT_DEFAULT_BRANCH}> + {defaultBranchLabel} + </SelectItem>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
auto-claude-ui/src/main/agent/agent-process.ts(2 hunks)auto-claude-ui/src/main/ipc-handlers/env-handlers.ts(3 hunks)auto-claude-ui/src/renderer/components/FileTreeItem.tsx(3 hunks)auto-claude-ui/src/renderer/components/TaskCreationWizard.tsx(13 hunks)auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx(6 hunks)auto-claude-ui/src/renderer/components/settings/sections/SectionRouter.tsx(1 hunks)auto-claude-ui/src/renderer/stores/release-store.ts(1 hunks)auto-claude-ui/src/shared/types/project.ts(1 hunks)auto-claude-ui/src/shared/types/task.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- auto-claude-ui/src/renderer/stores/release-store.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T15:01:23.017Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T15:01:23.017Z
Learning: Isolated builds use git worktrees with one branch per spec named `auto-claude/{spec-name}`, keeping all branches LOCAL until explicitly pushed
Applied to files:
auto-claude-ui/src/shared/types/project.tsauto-claude-ui/src/main/ipc-handlers/env-handlers.tsauto-claude-ui/src/shared/types/task.ts
🧬 Code graph analysis (2)
auto-claude-ui/src/main/agent/agent-process.ts (1)
auto-claude/task_logger/logger.py (1)
log(227-272)
auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx (3)
auto-claude-ui/src/shared/utils/debug-logger.ts (1)
debugLog(13-17)auto-claude-ui/src/renderer/components/ui/separator.tsx (1)
Separator(23-23)auto-claude-ui/src/renderer/components/ui/label.tsx (1)
Label(20-20)
🪛 GitHub Actions: Lint
auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx
[error] 77-77: TS2554: Expected 1-2 arguments, but got 4. (Occurred during 'pnpm typecheck' with 'tsc --noEmit')
🔇 Additional comments (26)
auto-claude-ui/src/renderer/components/FileTreeItem.tsx (4)
1-1: LGTM! Clean migration to native drag APIs.The import changes properly support native HTML5 drag-and-drop, replacing the @dnd-kit/core dependency.
73-73: LGTM! Appropriate state management.Local dragging state is correctly initialized and used for visual feedback.
119-121: LGTM! Clean state reset.The drag end handler correctly resets the dragging state.
125-127: LGTM! Proper native drag-and-drop integration.The draggable attribute and event handlers are correctly implemented, and the visual feedback using
isDraggingstate provides clear user feedback during drag operations.Also applies to: 131-131
auto-claude-ui/src/shared/types/task.ts (2)
223-225: LGTM!The optional
baseBranchfield inTaskMetadatacorrectly extends the type to support per-task worktree base branch configuration, aligning with the project-leveldefaultBranchsetting. This allows task-specific overrides when needed.
436-436: LGTM!The optional
baseBranchfield inTaskStartOptionsproperly enables overriding the base branch during worktree creation, complementing theTaskMetadata.baseBranchfield.auto-claude-ui/src/main/ipc-handlers/env-handlers.ts (3)
65-68: LGTM!The
DEFAULT_BRANCHwrite logic follows the established pattern for other environment variables.
116-122: LGTM!The new Git/Worktree settings section is well-documented with a helpful comment explaining the auto-detection fallback behavior.
230-234: LGTM!The
DEFAULT_BRANCHread logic correctly populatesconfig.defaultBranchfrom the parsed environment variables, consistent with other config fields.auto-claude-ui/src/shared/types/project.ts (1)
281-283: LGTM!The
defaultBranchproperty is appropriately typed and documented, completing the type definition that aligns with the env-handlers implementation.auto-claude-ui/src/renderer/components/settings/sections/SectionRouter.tsx (1)
172-172: LGTM!Correctly passes
projectPathto enable branch fetching functionality in the GitHubIntegration component.auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx (6)
37-37: LGTM!The new
projectPathprop is correctly typed as optional and properly destructured. This enables branch-related functionality while maintaining backward compatibility.Also applies to: 50-52
59-63: LGTM!Branch-related state is appropriately initialized for tracking branches, loading state, and errors.
87-127: LGTM!The
fetchBranchesfunction is well-implemented with:
- Early return guard when
projectPathis missing- Proper loading/error state management
- Auto-detection of default branch when not set
- Comprehensive debug logging
313-325: LGTM!The
BranchSelectoris conditionally rendered only whenprojectPathis available, and properly wired to updatedefaultBranchin the env config.
580-722: LGTM!The
BranchSelectorcomponent is well-implemented with:
- Search/filter functionality for branch discovery
- Auto-detect option that clears the selection
- Proper loading and error states
- Consistent styling with the existing
RepositoryDropdowncomponent- Clear user feedback showing the selected branch
100-100: SamedebugLogsignature issue.This call also passes more than 2 arguments. Consolidate into an object.
🔎 Proposed fix
- debugLog('fetchBranches: getGitBranches result:', { success: result.success, dataType: typeof result.data, dataLength: Array.isArray(result.data) ? result.data.length : 'N/A', error: result.error }); + debugLog('fetchBranches: getGitBranches result:', { + success: result.success, + dataType: typeof result.data, + dataLength: Array.isArray(result.data) ? result.data.length : 'N/A', + error: result.error + });Actually, reviewing this line more carefully - this call already uses an object as the second argument. The issue is on line 77, not here. This line appears correct.
auto-claude-ui/src/renderer/components/TaskCreationWizard.tsx (9)
2-2: LGTM: GitBranch icon import.The addition of the GitBranch icon is appropriate for the new Git options feature.
77-85: LGTM: Git options state management.The use of a placeholder value (
PROJECT_DEFAULT_BRANCH) to represent "use project default" is a reasonable workaround for Radix UI Select's constraints. The state structure is clean and well-documented.
337-449: LGTM: Enhanced drop handling for file references and images.The implementation correctly prioritizes file reference drops (via JSON data transfer) before falling back to image file drops. The cursor position handling for @mention insertion is appropriate, and the dependency array correctly includes
description.
517-518: LGTM: Correct filtering of placeholder value.The code correctly excludes the
PROJECT_DEFAULT_BRANCHplaceholder from the task metadata, ensuring only explicitly selected branches are sent to the backend.
554-554: LGTM: Proper Git state cleanup.The form reset correctly restores Git-related state to defaults, including both the
baseBranchvalue and theshowGitOptionsvisibility flag.
594-601: Review the hideCloseButton UX choice.The dialog hides its close button when the file explorer is open (
hideCloseButton={showFileExplorer}). While this might be intentional to encourage using the "Hide Files" button, users might find it confusing if they expect to close the dialog via the standard X button.Consider whether hiding the close button improves or hinders the user experience. If the intent is to prevent accidental closure, an alternative might be showing a confirmation dialog instead.
637-686: LGTM: Clean textarea wrapper with @mention highlighting.The simplified structure maintains the @mention syntax highlighting overlay while improving readability. The drag-over visual feedback is clear and appropriate.
186-186: All three Electron API methods (getGitBranches,getProjectEnv,detectMainBranch) are properly exposed in the preload script and return the expected{ success: boolean, data?: T }format. The implementations in the IPC handlers match their type signatures, and the usage in the component correctly handles the result format. No issues found.
517-518: The TaskMetadata type correctly includes the baseBranch field.The
TaskMetadatainterface inauto-claude-ui/src/shared/types/task.tsdefinesbaseBranch?: stringas an optional field with the purpose "Override base branch for this task's worktree". The code assignment at lines 517-518 is valid and properly typed.
auto-claude-ui/src/renderer/components/settings/integrations/GitHubIntegration.tsx
Outdated
Show resolved
Hide resolved
- Added useRef to manage custom drag image for better cleanup - Updated drag image positioning to prevent display issues - Enhanced cleanup process for drag image element on drag end This update refines the drag-and-drop experience within the file tree, ensuring that custom drag images are handled more effectively.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auto-claude-ui/src/renderer/components/GitHubSetupModal.tsx (2)
174-216: Skip behavior may lead to incomplete setup state.Both
GitHubOAuthFlow(line 190) andClaudeOAuthFlow(line 212) passonSkipas theironCancelcallback. If a user skips either authentication step, the modal closes immediately without tracking which authentications were completed. This could lead to:
- Proceeding with GitHub setup incomplete (can't access repositories)
- Proceeding with Claude setup incomplete (AI features won't work)
- No indication to the user about what functionality is unavailable
The setup flow should either:
- Prevent skipping critical auth steps, or
- Track completion state and warn users about limited functionality
🔎 Suggested approach
Option 1: Track partial completion and provide warnings
+ const [githubAuthSkipped, setGithubAuthSkipped] = useState(false); + const [claudeAuthSkipped, setClaudeAuthSkipped] = useState(false); + const handleSkipGithub = () => { + setGithubAuthSkipped(true); + // Decide: skip Claude too and go straight to end, or still prompt for Claude? + onSkip?.(); + }; + const handleSkipClaude = () => { + setClaudeAuthSkipped(true); + onSkip?.(); + }; case 'github-auth': return ( <> <DialogHeader> <DialogTitle className="flex items-center gap-2"> <Github className="h-5 w-5" /> Connect to GitHub </DialogTitle> <DialogDescription> Auto Claude requires GitHub to manage your code branches and keep tasks up to date. </DialogDescription> </DialogHeader> <div className="py-4"> <GitHubOAuthFlow onSuccess={handleGitHubAuthSuccess} - onCancel={onSkip} + onCancel={handleSkipGithub} /> </div> </> ); case 'claude-auth': return ( <> <DialogHeader> <DialogTitle className="flex items-center gap-2"> <Key className="h-5 w-5" /> Connect to Claude AI </DialogTitle> <DialogDescription> Auto Claude uses Claude AI for intelligent features like Roadmap generation, Task automation, and Ideation. </DialogDescription> </DialogHeader> <div className="py-4"> <ClaudeOAuthFlow onSuccess={handleClaudeAuthSuccess} - onCancel={onSkip} + onCancel={handleSkipClaude} /> </div> </> );Option 2: Make authentication steps mandatory (remove skip option for auth steps)
case 'github-auth': return ( <> {/* ... */} <div className="py-4"> <GitHubOAuthFlow onSuccess={handleGitHubAuthSuccess} - onCancel={onSkip} /> </div> </> );The choice depends on whether GitHub/Claude auth should be truly optional or required for the app to function.
406-455: Progress indicator doesn't reflect authentication progress.The current progress calculation (lines 418-422) maps three distinct authentication steps (
github-auth,claude-auth,repo) to the same progress index (0). This means users won't see any visual progress as they move through:
- GitHub authentication
- Claude authentication
- Repository detection
The progress indicator will appear stuck on step 1 until they reach branch selection, which is confusing and makes the UI feel unresponsive.
🔎 Recommended fix
Option 1: Add more granular progress steps
const renderProgress = () => { const steps: { label: string }[] = [ - { label: 'Authenticate' }, + { label: 'GitHub' }, + { label: 'Claude' }, { label: 'Configure' }, ]; // Don't show progress on complete step if (step === 'complete') return null; // Map steps to progress indices - // Auth steps (github-auth, claude-auth, repo) = 0 - // Config steps (branch) = 1 const currentIndex = step === 'github-auth' ? 0 : - step === 'claude-auth' ? 0 : - step === 'repo' ? 0 : - 1; + step === 'claude-auth' ? 1 : + step === 'repo' ? 2 : + 2; // branch stepOption 2: Keep two steps but advance after both auths complete
const renderProgress = () => { const steps: { label: string }[] = [ { label: 'Authenticate' }, { label: 'Configure' }, ]; // Don't show progress on complete step if (step === 'complete') return null; // Map steps to progress indices - // Auth steps (github-auth, claude-auth, repo) = 0 - // Config steps (branch) = 1 const currentIndex = - step === 'github-auth' ? 0 : - step === 'claude-auth' ? 0 : - step === 'repo' ? 0 : - 1; + step === 'github-auth' || step === 'claude-auth' ? 0 : + 1; // repo and branch stepsOption 1 provides better feedback but requires more UI space. Option 2 is simpler but still shows progress after both authentications complete.
♻️ Duplicate comments (3)
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (3)
234-279: Critical: Shell command injection vulnerability remains unresolved.This issue was flagged in a previous review and is still present. The commands are constructed using string interpolation without proper escaping of user-controlled inputs. Variables like
cwd,tempFile, andactiveProfile.configDirare wrapped in double quotes but not escaped, allowing special shell characters to be interpreted.For example, if
cwdcontainstest"; rm -rf / #, the command becomes:cd "test"; rm -rf / #" && claudeThis remains a critical security vulnerability that must be addressed before merging.
🔎 Recommended fix using proper shell escaping
// Helper function to escape shell arguments function escapeShellArg(arg: string): string { // Replace single quotes with '\'' and wrap in single quotes return `'${arg.replace(/'/g, "'\\''")}'`; }Then update the command construction:
-const cwdCommand = cwd ? `cd "${cwd}" && ` : ''; +const cwdCommand = cwd ? `cd ${escapeShellArg(cwd)} && ` : ''; // For temp file method (line 256): -const command = `clear && ${cwdCommand}source "${tempFile}" && rm -f "${tempFile}" && claude\r`; +const command = `clear && ${cwdCommand}source ${escapeShellArg(tempFile)} && rm -f ${escapeShellArg(tempFile)} && claude\r`; // For configDir method (line 263): -const command = `clear && ${cwdCommand}CLAUDE_CONFIG_DIR="${activeProfile.configDir}" claude\r`; +const command = `clear && ${cwdCommand}CLAUDE_CONFIG_DIR=${escapeShellArg(activeProfile.configDir)} claude\r`;
255-267: Security: Shell history exposure remains unresolved.This issue was flagged in a previous review and is still present. The commands at lines 256 and 263 expose sensitive file paths to shell history. The
clearcommand only clears the visible terminal output, not the shell history.Attackers with shell history access can:
- Reconstruct temp file name patterns and attempt to read before cleanup (race condition)
- Locate sensitive configuration directories
- Access these paths before cleanup executes
While the temp file uses secure permissions (0o600), the command execution path remains an exposure vector.
🔎 Recommended mitigations
Option 1 (Preferred): Pass environment variables directly to the spawned process instead of sourcing files via shell commands.
Option 2: Disable history for the command:
- Set
HISTCONTROL=ignorespace- Prefix commands with a space
- Or temporarily unset
HISTFILEExample:
const command = ` clear && ${cwdCommand}source "${tempFile}" && rm -f "${tempFile}" && claude\r`; // Note the leading space ^^^However, this relies on user shell configuration and is not foolproof. Option 1 is more secure.
375-389: Reliability: Arbitrary delays without verification remain unresolved.This issue was flagged in a previous review and is still present. The profile switching uses fixed 500ms delays (lines 380, 383) to wait for Claude to exit, but doesn't verify that Claude actually exited successfully.
On slower systems, under load, or with complex Claude sessions, these delays may be insufficient, causing:
- Commands to be sent to Claude instead of the shell
- Profile switch to appear successful but fail silently
- Race conditions in terminal state management
Consider implementing verification logic that waits for the shell prompt or checks terminal state before proceeding.
🔎 Example: Wait for shell prompt pattern
// Add a helper to wait for shell prompt async function waitForShellPrompt(terminal: TerminalProcess, timeoutMs = 5000): Promise<boolean> { return new Promise((resolve) => { const startTime = Date.now(); const checkInterval = setInterval(() => { const recentOutput = terminal.outputBuffer.slice(-200); if (recentOutput.match(/\$\s*$|\>\s*$/) || Date.now() - startTime > timeoutMs) { clearInterval(checkInterval); resolve(Date.now() - startTime < timeoutMs); } }, 100); }); } // Then in switchClaudeProfile: if (terminal.isClaudeMode) { terminal.pty.write('\x03'); await new Promise(resolve => setTimeout(resolve, 300)); terminal.pty.write('/exit\r'); const exitedSuccessfully = await waitForShellPrompt(terminal); if (!exitedSuccessfully) { console.warn('[ClaudeIntegration:switchClaudeProfile] Timeout waiting for Claude to exit'); return { success: false, error: 'Failed to exit Claude session' }; } }
🧹 Nitpick comments (3)
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (2)
122-172: Solid error handling, but consider minor cleanup.The new OAuth flow for non-profile terminals is well-structured with proper defensive checks and error event emission. However, there are a couple of minor improvements:
Line 165: The optional chaining
activeProfile?.idis unnecessary since lines 128-142 already handle the null case and return early.Code duplication: The IPC event sending blocks (lines 131-140, 150-158, 161-171) have similar structure and could be refactored into a helper function to reduce duplication.
🔎 Optional cleanup suggestions
1. Remove unnecessary optional chaining:
win.webContents.send(IPC_CHANNELS.TERMINAL_OAUTH_TOKEN, { terminalId: terminal.id, - profileId: activeProfile?.id, + profileId: activeProfile.id, email, success: false,2. Consider extracting a helper function:
function emitOAuthTokenEvent( win: Electron.BrowserWindow, terminalId: string, profileId: string | undefined, email: string | undefined, success: boolean, message?: string ): void { win.webContents.send(IPC_CHANNELS.TERMINAL_OAUTH_TOKEN, { terminalId, profileId, email, success, message, detectedAt: new Date().toISOString() } as OAuthTokenEvent); }
341-342: Consider using appropriate log levels.The "always-on tracing" uses
console.warnfor informational messages (lines 341-342, 358, 372, 376, 384, 387, 395, 406). While this ensures visibility,console.warnshould be reserved for actual warnings. Consider:
- Use
console.logfor informational tracing- Use
debugLogfor verbose debug information (already controlled by debug flag)- Reserve
console.warnfor genuine warnings or unexpected conditionsThis helps maintain log cleanliness and allows proper log level filtering in production.
🔎 Example adjustment
- console.warn('[ClaudeIntegration:switchClaudeProfile] Called for terminal:', terminal.id, '| profileId:', profileId); + console.log('[ClaudeIntegration:switchClaudeProfile] Called for terminal:', terminal.id, '| profileId:', profileId);Also applies to: 358-358, 372-372, 376-376, 384-384, 387-387, 395-395, 406-406
auto-claude-ui/src/renderer/components/project-settings/ClaudeOAuthFlow.tsx (1)
63-100: Consider adding timeout handling for long-running authentication.The
handleStartAuthfunction initiates authentication but has no timeout mechanism. IfinitializeClaudeProfilehangs or the user abandons the browser OAuth flow, the component will remain in the "authenticating" state indefinitely with no way to recover except refreshing the app.🔎 Suggested enhancement
Add a timeout to automatically transition to an error state if authentication takes too long:
+ const AUTH_TIMEOUT_MS = 300000; // 5 minutes const handleStartAuth = async () => { if (hasStartedRef.current) { console.warn('[ClaudeOAuth] Auth already started, ignoring duplicate call'); return; } hasStartedRef.current = true; console.warn('[ClaudeOAuth] Starting Claude authentication'); setStatus('authenticating'); setError(null); + // Set up timeout + const timeoutId = setTimeout(() => { + setError('Authentication timed out. Please try again.'); + setStatus('error'); + hasStartedRef.current = false; + }, AUTH_TIMEOUT_MS); try { // Get the active profile ID const profilesResult = await window.electronAPI.getClaudeProfiles(); if (!profilesResult.success || !profilesResult.data) { throw new Error('Failed to get Claude profiles'); } const activeProfileId = profilesResult.data.activeProfileId; console.warn('[ClaudeOAuth] Initializing profile:', activeProfileId); // Initialize the profile - this opens a terminal and runs 'claude setup-token' const result = await window.electronAPI.initializeClaudeProfile(activeProfileId); if (!result.success) { throw new Error(result.error || 'Failed to start authentication'); } console.warn('[ClaudeOAuth] Authentication started, waiting for token...'); + // Clear timeout on success + clearTimeout(timeoutId); // Status will be updated by the event listener when token is detected } catch (err) { + clearTimeout(timeoutId); console.error('[ClaudeOAuth] Authentication failed:', err); setError(err instanceof Error ? err.message : 'Authentication failed'); setStatus('error'); hasStartedRef.current = false; } };This gives users a way to recover if authentication stalls, improving the overall UX.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
auto-claude-ui/src/main/agent/agent-process.ts(2 hunks)auto-claude-ui/src/main/terminal/claude-integration-handler.ts(7 hunks)auto-claude-ui/src/renderer/App.tsx(1 hunks)auto-claude-ui/src/renderer/components/GitHubSetupModal.tsx(9 hunks)auto-claude-ui/src/renderer/components/project-settings/ClaudeOAuthFlow.tsx(1 hunks)auto-claude-ui/src/shared/types/project.ts(1 hunks)auto-claude-ui/src/shared/types/settings.ts(1 hunks)auto-claude-ui/src/shared/types/task.ts(2 hunks)auto-claude/cli/workspace_commands.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- auto-claude-ui/src/renderer/App.tsx
- auto-claude/cli/workspace_commands.py
🚧 Files skipped from review as they are similar to previous changes (4)
- auto-claude-ui/src/main/agent/agent-process.ts
- auto-claude-ui/src/shared/types/task.ts
- auto-claude-ui/src/shared/types/settings.ts
- auto-claude-ui/src/shared/types/project.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-19T15:01:23.017Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T15:01:23.017Z
Learning: Set up OAuth token by running `claude setup-token` and adding `CLAUDE_CODE_OAUTH_TOKEN=your-token` to `auto-claude/.env`
Applied to files:
auto-claude-ui/src/renderer/components/project-settings/ClaudeOAuthFlow.tsxauto-claude-ui/src/main/terminal/claude-integration-handler.ts
📚 Learning: 2025-12-19T15:01:23.017Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T15:01:23.017Z
Learning: Security profile is cached in `.auto-claude-security.json` with three-layer defense: OS Sandbox, Filesystem Permissions, and Command Allowlist from project analysis
Applied to files:
auto-claude-ui/src/main/terminal/claude-integration-handler.ts
🧬 Code graph analysis (2)
auto-claude-ui/src/renderer/components/project-settings/ClaudeOAuthFlow.tsx (1)
auto-claude-ui/src/renderer/components/ui/card.tsx (1)
CardContent(58-58)
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (3)
auto-claude-ui/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(531-536)auto-claude-ui/src/main/terminal/types.ts (1)
OAuthTokenEvent(35-42)auto-claude-ui/src/shared/utils/debug-logger.ts (2)
debugLog(13-17)debugError(25-29)
🔇 Additional comments (5)
auto-claude-ui/src/main/terminal/claude-integration-handler.ts (4)
13-13: LGTM! Good addition for enhanced observability.The debug utility imports are appropriate and will help with troubleshooting. The debug-logger properly guards logs with
isDebugEnabled(), ensuring minimal performance impact in production.
96-121: LGTM! Improved pattern matching and proper error handling.The updated regex pattern now correctly matches both custom profiles (
profile-\d+) and the default profile. The OAuth token handling for profile-specific terminals includes appropriate success/failure event emission.
207-232: Good debug logging for profile resolution.The enhanced debug logging provides excellent visibility into the profile resolution logic, including token presence and default status. This will significantly aid in troubleshooting profile-related issues.
344-364: Excellent debug logging for profile switching flow.The enhanced debug logging provides comprehensive visibility into the profile switching process, including terminal state, profile resolution, and command execution. This will significantly aid in troubleshooting profile switching issues.
Also applies to: 391-408
auto-claude-ui/src/renderer/components/GitHubSetupModal.tsx (1)
153-158: Verify Claude token persistence before proceeding.The comment on line 155 assumes the OAuth flow has successfully saved the token, but there's no explicit verification. If the token save fails silently in the background, users may proceed with an incomplete setup and encounter cryptic errors later when AI features are used.
Consider adding explicit verification:
🔎 Suggested verification
// Handle Claude OAuth success const handleClaudeAuthSuccess = async () => { - // Claude token is already saved to active profile by the OAuth flow + // Verify Claude token was saved successfully + try { + const profilesResult = await window.electronAPI.getClaudeProfiles(); + if (!profilesResult.success || !profilesResult.data?.activeProfileId) { + setError('Failed to verify Claude authentication'); + setStep('claude-auth'); + return; + } + } catch (err) { + setError('Failed to verify Claude token'); + setStep('claude-auth'); + return; + } + // Move to repo detection await detectRepository(); };Alternatively, if the token verification is expensive or would require additional IPC calls, document this assumption more clearly and ensure the OAuth flow component has robust error handling.
auto-claude-ui/src/renderer/components/project-settings/ClaudeOAuthFlow.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
auto-claude-ui/src/renderer/components/FileTreeItem.tsx(3 hunks)
🔇 Additional comments (3)
auto-claude-ui/src/renderer/components/FileTreeItem.tsx (3)
1-1: LGTM! Clean migration to native React APIs.The import changes correctly replace the external DnD Kit dependency with native React hooks and event types.
73-74: LGTM! Appropriate state management.The state declarations correctly support the native drag-and-drop implementation with local dragging state and a ref for the custom drag image element.
131-137: LGTM! Native drag-and-drop correctly integrated.The JSX changes properly wire up native HTML5 drag-and-drop with the
draggableattribute and event handlers. The conditional styling provides clear visual feedback during drag operations.
- Introduced shell escape utilities to prevent command injection in terminal commands. - Updated terminal handlers to use safe command construction for profile switching and OAuth token initialization. - Removed unnecessary console warnings, replacing them with debug logs for cleaner output. - Implemented a wait mechanism to monitor terminal output for Claude exit, improving profile switching reliability. This update improves the security and reliability of terminal command execution, ensuring user inputs are safely handled.
…and ClaudeOAuthFlow components - Added useEffect in FileTreeItem to clean up custom drag image on component unmount, preventing memory leaks. - Enhanced drag image creation using safe DOM manipulation instead of innerHTML. - Updated ClaudeOAuthFlow to manage auto-advance timeout with cleanup on unmount, ensuring onSuccess is not called after component unmount. These changes enhance the reliability and performance of drag-and-drop functionality and OAuth flow handling.
2 participants
Summary
Type of Change
Related Issues
Changes Made
Screenshots
Checklist
pre-commit run --all-filesand fixed any issuesTesting
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.