Fixed worktree duplication on creation#411
Merged
pedramamini merged 5 commits intoRunMaestro:mainfrom Feb 19, 2026
Merged
Conversation
- Fix path normalization bug in worktree deduplication check (line 740) The final dedup check was comparing unnormalized paths, causing failures on Windows where paths might use different separators or formatting. Now uses the already-normalized path variable for consistent comparison. - Add platform-specific label for reveal in explorer action TabBar and FileExplorerPanel components now detect platform and show 'Reveal in Explorer' on Windows or 'Reveal in Finder' on macOS/Linux. Uses window.maestro.os.platform for detection. - Update tests to accept platform-specific labels Test expectations now use regex patterns /Reveal in (Finder|Explorer)/ to work correctly on both Windows and Unix platforms. All 323 affected unit tests pass.
- Phase 1: Add OS API (getPlatform, getArch) via IPC for cross-platform support - Created createOsApi() in preload/system.ts - Added IPC handlers os:getPlatform and os:getArch in ipc/handlers/system.ts - Exposed os namespace in contextBridge (preload/index.ts) - Added OS API type to global.d.ts for TypeScript support - Phase 2: Update components to use new OS API - Created useOsPlatform hook to cache platform detection and handle test environment - Updated FileExplorerPanel.tsx to use useOsPlatform instead of direct window.maestro.os.platform - Updated TabBar.tsx FileTab component to use useOsPlatform for reveal label - Phase 3: Fix path normalization in worktree handlers - Added normalizePath() helper to convert backslashes to forward slashes and remove trailing slashes - Updated startup scan dedup (line 653): normalize paths when building currentPaths Set - Updated file watcher discovery (line 711-720): use normalizePath for Windows path comparison - Updated file watcher dedup (line 740): normalize paths in duplicate check - Updated legacy scanner dedup (line 859-860): normalize paths for cross-platform compatibility Fixes: - Windows platform detection now works correctly (was undefined before) - Duplicate worktree entries on Windows are now prevented via consistent path normalization - Supports both backslash (Windows) and forward slash (Unix) path separators
…cates on Windows - Normalize paths when checking for existing sessions at line 321 (quick create worktree) - Normalize paths when checking for existing sessions at line 826 (legacy scanner) - Ensures backslash/forward slash and trailing slash mismatches don't create duplicate entries - This fixes the issue where Windows paths with backslashes weren't matching normalized forward slash paths
…reation The recentlyCreatedWorktreePathsRef guard was populated after awaiting worktreeSetup + fetchGitInfo. If fetchGitInfo took longer than chokidar's 500ms debounce, the watcher event arrived while the ref was still empty, bypassing the dedup check and creating a second session entry. Fix: mark the path in the ref BEFORE calling worktreeSetup, and clean it up if creation fails. Also normalize paths (backslashes → forward slashes, collapse //) before storing and checking, so chokidar's Windows backslash paths match the renderer's forward-slash paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR introduces platform-aware UI labeling and fixes critical worktree duplication bugs. The changes add a new The worktree handling improvements address two important bugs:
Both changes improve cross-platform consistency and reliability. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (UI)
participant H as useOsPlatform Hook
participant P as Preload Bridge
participant M as Main Process
participant OS as Node process.platform
Note over R,OS: Platform Detection Flow
R->>H: Call useOsPlatform()
H->>H: Check cachedPlatform
alt Platform cached
H-->>R: Return cached value
else Not cached
H->>P: window.maestro.os.getPlatform()
P->>M: ipcRenderer.invoke('os:getPlatform')
M->>OS: process.platform
OS-->>M: 'darwin' | 'win32' | 'linux'
M-->>P: platform string
P-->>H: platform string
H->>H: Cache result
H-->>R: Return platform
end
Note over R,OS: Worktree Creation Race Condition Fix
participant WT as Worktree Handler
participant REF as recentlyCreatedPathsRef
participant GIT as Git Service
participant FW as File Watcher
WT->>WT: normalizePath(worktreePath)
WT->>REF: Add normalized path BEFORE creation
WT->>GIT: Create worktree on disk
GIT-->>WT: Success
par Async operations
WT->>GIT: fetchGitInfo()
and
FW->>FW: Detects new directory
FW->>REF: Check if path in ref
REF-->>FW: Yes (marked as recently created)
FW->>FW: Skip duplicate creation
end
WT->>WT: Create session
Note over WT: After 10s timeout, remove from ref
Last reviewed commit: 7193964 |
📝 WalkthroughWalkthroughThe changes introduce OS platform and architecture detection via IPC handlers and preload APIs, exposing these capabilities to the renderer. UI components are updated to display platform-aware labels ("Reveal in Finder" on macOS/Linux, "Reveal in Explorer" on Windows). Tests are relaxed to accept both platform variants. Path normalization is improved in worktree handlers for cross-platform consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as React Component
participant Hook as useOsPlatform Hook
participant IPC as ipcRenderer
participant Main as Main Process Handler
participant OS as Node.js process
Renderer->>Hook: useOsPlatform()
activate Hook
alt Platform already cached
Hook-->>Renderer: Return cached platform
else First call
Hook->>IPC: ipcRenderer.invoke('os:getPlatform')
activate IPC
IPC->>Main: os:getPlatform handler
activate Main
Main->>OS: process.platform
OS-->>Main: 'darwin' | 'win32' | 'linux'
Main-->>IPC: platform string
deactivate Main
IPC-->>Hook: platform string
deactivate IPC
Hook->>Hook: Cache result
Hook-->>Renderer: Return platform
end
deactivate Hook
Renderer->>Renderer: Render platform-aware label<br/>(win32 → 'Explorer', else → 'Finder')
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/renderer/hooks/worktree/useWorktreeHandlers.ts (3)
872-874:⚠️ Potential issue | 🟠 MajorReport legacy scanner errors to Sentry.
Consistent with the other scan operations, errors here should be reported to Sentry for tracking.
As per coding guidelines: "Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
🐛 Suggested fix
} catch (error) { - console.error(`[WorktreeScanner] Error scanning ${session.worktreeParentPath}:`, error); + captureException(error, { + extra: { worktreeParentPath: session.worktreeParentPath, sessionId: session.id } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 872 - 874, Replace the console.error in the legacy scanner catch block with Sentry reporting: import and call captureException(error) from 'src/utils/sentry' and add a contextual captureMessage or tags that include session.worktreeParentPath (and any relevant session id) so the exception is recorded with context; update the catch in useWorktreeHandlers.ts (the block handling the legacy scanner error) to call captureMessage("WorktreeScanner error scanning <path>") and captureException(error) using session.worktreeParentPath for the message/tags.
359-361:⚠️ Potential issue | 🟠 MajorReport scan errors to Sentry instead of only logging to console.
This catch block swallows the exception with
console.error, which violates the coding guidelines. While the error is recoverable (the app continues), it should be reported to Sentry for tracking.As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks" and "Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
🐛 Suggested fix
+import { captureException } from '../../utils/sentry'; // ... at the catch block: } catch (err) { - console.error('Failed to scan for worktrees:', err); + captureException(err, { extra: { basePath: config.basePath } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 359 - 361, Replace the console.error in the catch inside useWorktreeHandlers (the scan-for-worktrees try/catch) with a Sentry report: import captureException (and/or captureMessage) from 'src/utils/sentry' and call captureException(err, { extra: { context: 'Failed to scan for worktrees', /* include local vars like repoPath, workspaceId, scanOptions if in scope */ } }); keep the console.error only if you want local logging, but ensure the exception is sent to Sentry with contextual metadata so the error is tracked.
658-663:⚠️ Potential issue | 🟠 MajorReport startup scan errors to Sentry.
Same issue as in
handleSaveWorktreeConfig—scan errors should be reported to Sentry for tracking rather than only logged to console.As per coding guidelines: "Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
🐛 Suggested fix
} catch (err) { - console.error( - `[WorktreeStartup] Error scanning ${parentSession.worktreeConfig!.basePath}:`, - err - ); + captureException(err, { + extra: { basePath: parentSession.worktreeConfig!.basePath, sessionId: parentSession.id } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 658 - 663, The catch block that currently only console.errors scan failures for "[WorktreeStartup] Error scanning ${parentSession.worktreeConfig!.basePath}" should also report the error to Sentry: import captureException (and optional captureMessage) from 'src/utils/sentry' and call captureException(err, { extra: { basePath: parentSession.worktreeConfig!.basePath, sessionId: parentSession.id } }) (or captureMessage with the same context) inside the catch so the scan error is tracked; reference the catch in useWorktreeHandlers.ts around the WorktreeStartup scan (look for the console.error with parentSession.worktreeConfig!.basePath) and mirror the same Sentry reporting used in handleSaveWorktreeConfig.
🧹 Nitpick comments (1)
src/renderer/hooks/worktree/useWorktreeHandlers.ts (1)
422-441: Race prevention is sound; consider cleanup in catch block for consistency.The approach of marking the path before the async operation prevents the file watcher race condition effectively. The 10-second timeout provides a safety net for all failure modes.
However, the explicit cleanup at line 440 only handles the
!result.successcase. IfworktreeSetupthrows an exception, the catch block (lines 470-477) doesn't clean up the path—it relies solely on the timeout. For consistency with the stated intent ("remove from ref so the path isn't permanently blocked"), consider adding cleanup in the catch block as well.♻️ Suggested improvement
} catch (err) { + // Clean up the path from ref on any failure + recentlyCreatedWorktreePathsRef.current.delete(normalizedCreatedPath); console.error('[WorktreeConfig] Failed to create worktree:', err); notifyToast({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 422 - 441, The code marks paths in recentlyCreatedWorktreePathsRef (via normalizePath) before calling window.maestro.git.worktreeSetup to avoid watcher races, but only explicitly removes the path when result.success is false; modify the catch block that handles exceptions from worktreeSetup to also remove the normalizedCreatedPath from recentlyCreatedWorktreePathsRef.current (same cleanup as in the !result.success branch) so the path isn't permanently blocked if worktreeSetup throws, then proceed with the existing error handling/logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/useOsPlatform.ts`:
- Around line 36-46: Replace the console.error usage and silent swallow in the
catch of the anonymous async IIFE that calls window.maestro.os.getPlatform():
import and call Sentry's captureException (and optionally captureMessage) from
src/utils/sentry inside the catch, passing the caught error and contextual info
(e.g., that getPlatform failed), then continue the existing fallback behavior
(keeping the isMounted check and not rethrowing if recoverable); update
references around cachedPlatform and setPlatform as needed so the fallback still
runs when getPlatform fails.
---
Outside diff comments:
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 872-874: Replace the console.error in the legacy scanner catch
block with Sentry reporting: import and call captureException(error) from
'src/utils/sentry' and add a contextual captureMessage or tags that include
session.worktreeParentPath (and any relevant session id) so the exception is
recorded with context; update the catch in useWorktreeHandlers.ts (the block
handling the legacy scanner error) to call captureMessage("WorktreeScanner error
scanning <path>") and captureException(error) using session.worktreeParentPath
for the message/tags.
- Around line 359-361: Replace the console.error in the catch inside
useWorktreeHandlers (the scan-for-worktrees try/catch) with a Sentry report:
import captureException (and/or captureMessage) from 'src/utils/sentry' and call
captureException(err, { extra: { context: 'Failed to scan for worktrees', /*
include local vars like repoPath, workspaceId, scanOptions if in scope */ } });
keep the console.error only if you want local logging, but ensure the exception
is sent to Sentry with contextual metadata so the error is tracked.
- Around line 658-663: The catch block that currently only console.errors scan
failures for "[WorktreeStartup] Error scanning
${parentSession.worktreeConfig!.basePath}" should also report the error to
Sentry: import captureException (and optional captureMessage) from
'src/utils/sentry' and call captureException(err, { extra: { basePath:
parentSession.worktreeConfig!.basePath, sessionId: parentSession.id } }) (or
captureMessage with the same context) inside the catch so the scan error is
tracked; reference the catch in useWorktreeHandlers.ts around the
WorktreeStartup scan (look for the console.error with
parentSession.worktreeConfig!.basePath) and mirror the same Sentry reporting
used in handleSaveWorktreeConfig.
---
Nitpick comments:
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 422-441: The code marks paths in recentlyCreatedWorktreePathsRef
(via normalizePath) before calling window.maestro.git.worktreeSetup to avoid
watcher races, but only explicitly removes the path when result.success is
false; modify the catch block that handles exceptions from worktreeSetup to also
remove the normalizedCreatedPath from recentlyCreatedWorktreePathsRef.current
(same cleanup as in the !result.success branch) so the path isn't permanently
blocked if worktreeSetup throws, then proceed with the existing error
handling/logging.
src/renderer/hooks/useOsPlatform.ts
Outdated
Comment on lines
36
to
46
| (async () => { | ||
| try { | ||
| const result = await window.maestro.os.getPlatform(); | ||
| if (isMounted) { | ||
| cachedPlatform = result; | ||
| setPlatform(result); | ||
| } | ||
| } catch (error) { | ||
| // Fallback if IPC call fails | ||
| console.error('Failed to get platform:', error); | ||
| if (isMounted) { |
There was a problem hiding this comment.
Replace console.error with Sentry capture and avoid silent swallowing.
The catch block logs to console and suppresses the error. Please report via src/utils/sentry and keep the fallback behavior if this is recoverable.
As per coding guidelines: “Do NOT silently swallow exceptions with try-catch-console.error blocks… Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context.”
Proposed fix
-import { useEffect, useState } from 'react';
+import { useEffect, useState } from 'react';
+import { captureException } from 'src/utils/sentry';
@@
(async () => {
try {
const result = await window.maestro.os.getPlatform();
if (isMounted) {
cachedPlatform = result;
setPlatform(result);
}
} catch (error) {
- // Fallback if IPC call fails
- console.error('Failed to get platform:', error);
+ captureException(error);
if (isMounted) {
setPlatform('unknown');
}
}
})();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (async () => { | |
| try { | |
| const result = await window.maestro.os.getPlatform(); | |
| if (isMounted) { | |
| cachedPlatform = result; | |
| setPlatform(result); | |
| } | |
| } catch (error) { | |
| // Fallback if IPC call fails | |
| console.error('Failed to get platform:', error); | |
| if (isMounted) { | |
| import { useEffect, useState } from 'react'; | |
| import { captureException } from 'src/utils/sentry'; | |
| (async () => { | |
| try { | |
| const result = await window.maestro.os.getPlatform(); | |
| if (isMounted) { | |
| cachedPlatform = result; | |
| setPlatform(result); | |
| } | |
| } catch (error) { | |
| captureException(error); | |
| if (isMounted) { | |
| setPlatform('unknown'); | |
| } | |
| } | |
| })(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/useOsPlatform.ts` around lines 36 - 46, Replace the
console.error usage and silent swallow in the catch of the anonymous async IIFE
that calls window.maestro.os.getPlatform(): import and call Sentry's
captureException (and optionally captureMessage) from src/utils/sentry inside
the catch, passing the caught error and contextual info (e.g., that getPlatform
failed), then continue the existing fallback behavior (keeping the isMounted
check and not rethrowing if recoverable); update references around
cachedPlatform and setPlatform as needed so the fallback still runs when
getPlatform fails.
pedramamini
reviewed
Feb 18, 2026
Collaborator
pedramamini
left a comment
pedramamini
left a comment
There was a problem hiding this comment.
Review Feedback
Two separate concerns bundled here. The worktree fix is solid; the platform detection needs rework.
Feature 1: Platform-aware "Reveal in Finder/Explorer" — Needs Rework
Over-engineered for a static value. process.platform never changes at runtime. Adding async IPC handlers (os:getPlatform, os:getArch), a new hook with caching/error-handling, and useEffect + useState for something that could be a synchronous constant is unnecessary complexity.
Specific issues:
-
'unknown'flash on first render. The hook returns'unknown'before the IPC resolves. Users briefly see the wrong label (or no label match) until the async call completes. A synchronous value from preload avoids this entirely. -
Missing Linux.
revealLabelonly handleswin32→ "Explorer" with fallback to "Finder". Linux users see "Reveal in Finder" which is incorrect — should be "Reveal in File Manager" or "Show in Files". -
Duplicated
useMemo. The samerevealLabelmemo is copy-pasted into bothFileExplorerPanel.tsxandTabBar.tsx. Should be a single shared utility. -
Redundant type exports.
OsApi(interface) andOsApiType(ReturnType) export the same shape. Pick one.
Suggested approach: Expose process.platform as a synchronous property in preload (e.g., window.maestro.platform), then use a simple utility function like getRevealLabel(platform: string): string in a shared location. No hook needed.
Feature 2: Worktree path normalization + race fix — LGTM
This is the real value in the PR:
normalizePath()correctly centralizes the scattered.replace(/\/+$/, '')calls and adds backslash handling for Windows- Moving
recentlyCreatedWorktreePathsRef.add()beforegit.worktreeSetup()correctly closes the chokidar race window — good catch - Cleanup on failure (removing from ref if creation fails) is handled correctly
- All path comparisons now consistently use
normalizePath()
Minor nit: normalizePath is local to useWorktreeHandlers.ts. If path normalization is needed elsewhere later, consider extracting to a shared util. Fine as-is for now.
Recommendation
Split into two PRs:
- Worktree path normalization — merge as-is
- Platform-aware labels — rework with synchronous preload value + shared utility + Linux support
…ranch) Platform-aware Reveal in Finder/Explorer changes are being reworked per reviewer feedback (synchronous preload value, Linux support, shared util). Extracted to separate branch: fix-reveal-label. This branch now contains only the worktree deduplication fixes: - normalizePath helper for cross-platform path comparison - Race condition fix: mark paths before async git operations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
chr1syy
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces more robust handling of Git worktree session creation to avoid duplicate sessions due to path normalization and race conditions.
Worktree session handling improvements:
Adds a
normalizePathutility to consistently compare file paths by converting backslashes to slashes and removing redundant/trailing slashes. This prevents duplicate Git worktree sessions when paths differ only in formatting. [1] [2] [3]Ensures that paths are marked as "recently created" before starting asynchronous worktree creation, and that these marks are removed if creation fails. This prevents race conditions where the file watcher could create duplicate sessions for the same path. [1] [2] [3] [4] [5] [6]