Fix Windows compatibility and critical UI bugs in prompts view #23
Conversation
…tibility - Renamed 'chatmodes' to 'agents' throughout UI to match VS Code terminology - Added support for legacy chatmodes/ and chatmode/ (singular) directory paths - Changed syncInstructions default to true for better UX - Fixed Windows symlink handling with automatic fallback to file copy - Enhanced debug logging throughout sync and UI components - Improved cross-platform path handling (Windows backslash support) - Updated documentation to reflect agents terminology and Windows compatibility - Condensed CHANGELOG for cleaner release notes Breaking Changes: - None (maintains backward compatibility with legacy chatmode paths) Fixes: - Fixed UI not displaying agents (was filtering for 'chatmode' type) - Fixed instructions not syncing by default - Fixed Windows path separator issues in symlink detection - Fixed activate/deactivate button behavior in webview - Fixed repository storage to use globalStorage instead of User/prompts
WalkthroughAdds repository-backed prompt storage under globalStorage, supports Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"primaryColor":"#0b5cff","secondaryColor":"#EAF2FF","noteBackground":"#F7FAFF"}}}%%
sequenceDiagram
participant User
participant Webview
participant CmdMgr as CommandManager
participant SyncMgr as SyncManager
participant RepoStore as RepoStorage
participant FS as FileSystem
Note over User,Webview: User triggers Activate for a prompt
User->>Webview: Click Activate
Webview->>CmdMgr: toggleSelection(promptPath, repositoryUrl)
CmdMgr->>Webview: optimistic UI update
CmdMgr->>SyncMgr: activatePrompt(promptPath, repositoryUrl)
activate SyncMgr
SyncMgr->>RepoStore: getRepositoryStorageDirectory(context)
RepoStore-->>SyncMgr: repo storage base path
SyncMgr->>SyncMgr: compute unique workspaceName(repository, filename)
SyncMgr->>FS: check symlink capability
alt Symlink allowed
SyncMgr->>FS: create symlink(repoFile → prompts/workspaceName)
else Symlink unavailable (Windows fallback)
SyncMgr->>FS: copy file(repoFile → prompts/workspaceName)
end
FS-->>SyncMgr: success (workspaceName)
SyncMgr-->>CmdMgr: returned workspaceName
deactivate SyncMgr
CmdMgr->>Webview: refresh view (apply final state)
Webview-->>User: Activation completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses critical Windows compatibility issues and UI bugs in the prompts view functionality. The changes focus on proper path handling across platforms, fixing broken button behaviors, and improving bulk operations with better user feedback.
Key Changes:
- Implements Windows-specific symlink fallback to file copying when admin/developer mode is unavailable
- Refactors bulk activate/deactivate operations to work directly with file system operations and show progress notifications
- Adds automatic duplicate repository detection and removal with configuration persistence
- Implements sentinel value pattern for dropdown filtering to properly distinguish between "all sources" and "no sources selected" states
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/promptTreeProvider.ts | Major refactoring to handle Windows file copies alongside symlinks, added workspace name calculation for duplicate filename resolution, and improved active prompt detection across platforms |
| src/ui/promptDetailsWebview.ts | Fixed inactive prompt path resolution on Windows by prioritizing repository storage lookup and adding detailed debugging output for path resolution issues |
| src/ui/promptCommands.ts | Refactored bulk operations (selectAll/deselectAll) to directly manage symlinks with progress notifications and proper error handling; improved toggleSelection with state management |
| src/ui/promptCardsWebview.ts | Added path escaping for Windows backslashes in JavaScript strings, implemented sentinel value pattern for dropdown filtering, and enhanced logging for webview message handling |
| src/syncManager.ts | Implemented Windows symlink fallback to file copy with EPERM handling, added unique workspace name generation for conflicting filenames, centralized repository identifier extraction logic |
| src/storage/repositoryStorage.ts | Added getRepositoryStorageDirectory helper function with platform-specific paths and extension context support for reliable global storage location |
| src/extension.ts | Updated PromptDetailsWebviewProvider initialization to pass extension context for improved path resolution |
| src/constant.ts | Renamed REPO_SYNC_CHAT_MODE_PATH from chatmodes/ to agents/, added legacy path constants for backward compatibility |
| src/configManager.ts | Implemented automatic duplicate repository removal with configuration updates, refactored getPromptsDirectory to properly handle VS Code profiles |
| package.json | Version bump to 1.5.2, updated setting descriptions to reflect agents/ naming and legacy chatmodes/ support |
| media/main.css | Added type-agents CSS class for styling consistency with naming changes |
| README.md | Updated documentation to explain Windows symlink behavior, file copy fallback, and legacy directory support |
| CHANGELOG.md | Added release notes documenting all bug fixes and improvements |
| .github/copilot-instructions.md | Updated architecture documentation to reflect new storage patterns and Windows-specific behaviors |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/configManager.ts (1)
45-72: Side-effect in getter with potential race condition.The
repositoriesgetter triggers an asynchronous configuration update as a side effect. This has two concerns:
Unexpected mutation: Getters are typically expected to be pure reads. Modifying global configuration as a side effect can surprise callers.
Race condition: If
repositoriesis accessed multiple times in quick succession (e.g., during UI refresh), each call will detect duplicates and attempt to update the configuration, potentially causing redundant updates or race conditions.Consider moving the deduplication logic to a dedicated method called explicitly during extension activation or sync operations.
🔎 Suggested refactor
+ /** + * Deduplicate repositories and persist if needed. Call this explicitly during init/sync. + */ + async deduplicateRepositories(): Promise<void> { + const repository = vscode.workspace.getConfiguration('promptitude').get<string[]>('repositories', []); + const sanitized = repository.map(r => (r ?? '').trim()).filter(r => r.length > 0); + const uniqueArray = Array.from(new Set(sanitized)); + + if (uniqueArray.length !== repository.length) { + this.logger.info(`Removing ${repository.length - uniqueArray.length} duplicate repository URL(s)`); + try { + await vscode.workspace.getConfiguration('promptitude') + .update('repositories', uniqueArray, vscode.ConfigurationTarget.Global); + vscode.window.showInformationMessage( + `Removed ${repository.length - uniqueArray.length} duplicate repository URL(s).` + ); + } catch (error) { + this.logger.error('Failed to update repositories configuration', error as Error); + vscode.window.showWarningMessage('Found duplicates but failed to update. Please remove manually.'); + } + } + } get repositories(): string[] { const repository = vscode.workspace.getConfiguration('promptitude').get<string[]>('repositories', []); - const sanitized = repository - .map(r => (r ?? '').trim()) - .filter(r => r.length > 0); - const uniqueArray = Array.from(new Set(sanitized)); - - // If duplicates were found, update the configuration to remove them. - if (uniqueArray.length !== repository.length) { - // ... async update logic - } - - return uniqueArray; + return Array.from(new Set( + repository.map(r => (r ?? '').trim()).filter(r => r.length > 0) + )); }src/ui/promptCardsWebview.ts (1)
1013-1019:getTypeIconfunction still uses 'chatmode' instead of 'agents'.The function returns the icon for
case 'chatmode'but the rest of the code now uses'agents'as the type. This may cause agents to fall through to the default case.🔎 Proposed fix
function getTypeIcon(type) { switch(type) { - case 'chatmode': return '💬'; + case 'agents': return '🤖'; case 'instructions': return '📖'; case 'prompts': return '⚡'; default: return '📄'; } }src/syncManager.ts (1)
1001-1024: URL decoding inextractRepositoryUrlFromTargetPathdoes not match the base64url encoding.The function encodes URLs using proper
base64urlviaBuffer.from(url, 'utf8').toString('base64url'), but decoding only performs simple string replacements (replacing_with/and prependinghttps://). This skips the actual base64url decoding step and will fail to recover any URLs containing characters that differ between standard Base64 and base64url encoding (such as+,-,=).The symlink recovery logic depends on this function—broken URL extraction will prevent proper symlink restoration. Use proper base64url decoding or align the encoding method with the decoding logic.
src/ui/promptCommands.ts (2)
29-39: Inconsistent async handling in command registrations.
toggleSelectionCommand(line 29) and other commands (lines 42-56) are properly async, butselectAllCommand(line 33) anddeselectAllCommand(line 37) are not awaiting their async handlers. This could lead to unhandled promise rejections.Suggested fix
- const selectAllCommand = vscode.commands.registerCommand('prompts.selectAll', () => { - this.selectAll(); + const selectAllCommand = vscode.commands.registerCommand('prompts.selectAll', async () => { + await this.selectAll(); }); - const deselectAllCommand = vscode.commands.registerCommand('prompts.deselectAll', () => { - return this.deselectAll(); + const deselectAllCommand = vscode.commands.registerCommand('prompts.deselectAll', async () => { + await this.deselectAll(); });
179-192: Potential double-revert on activation/deactivation failure.When
activationErrorordeactivationErroris caught (lines 133-138 or 162-166), the code reverts the toggle and then re-throws the error. The outer catch block (lines 183-191) then catches this re-thrown error and attempts another revert. This could cause the prompt to end up in the wrong state.Suggested fix - track whether revert already happened
} catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); this.logger.error(`Failed to toggle selection: ${errorMsg}`, error instanceof Error ? error : undefined); vscode.window.showErrorMessage(`Failed to toggle selection: ${errorMsg}`); - // Try to revert the toggle - try { - const promptInfo = this.getPromptInfo(item); - if (promptInfo) { - this.treeProvider.toggleSelection(promptInfo); - } - } catch { - // Ignore revert errors - } + // Note: revert already handled in inner catch blocks for activation/deactivation errors }
🧹 Nitpick comments (4)
src/storage/repositoryStorage.ts (1)
45-94: Hardcoded extension ID may drift from package.json.The fallback path uses a hardcoded extension ID
'logientnventive.promptitude-extension'which is derived frompublisher.namein package.json. If those values change, this will silently use a different directory.Consider extracting the extension ID as a constant or reading it dynamically when context is unavailable.
🔎 Suggested approach
// Option 1: Define as a shared constant in a central location export const EXTENSION_ID = 'logientnventive.promptitude-extension'; // Option 2: In the function, derive from package.json values if available // However, since this is the fallback path (context unavailable), a constant is acceptableThe overall implementation is solid with proper platform-specific path handling.
src/ui/promptDetailsWebview.ts (1)
110-147: Inconsistent use of rawfsmodule instead ofFileSystemManager.The method uses
require('fs')directly while the class has aFileSystemManagerinstance (this.fileSystem). This creates inconsistency in error handling and testing patterns.Consider using async methods from
FileSystemManageror at minimum, consolidating the fs import at the module level rather than inlinerequire().🔎 Suggested approach
private getActualFilePath(prompt: PromptInfo): string { - const fs = require('fs'); + // Note: This method is synchronous due to how it's called. + // Consider making it async and using this.fileSystem methods. + const fs = require('fs');Alternatively, refactor to async:
private async getActualFilePath(prompt: PromptInfo): Promise<string> { // Use this.fileSystem.fileExists() instead of fs.existsSync() // Use this.fileSystem.readDirectory() instead of fs.readdirSync() }src/syncManager.ts (1)
3-3: Unused import:osmodule.The
osmodule is imported but doesn't appear to be used in this file. The repository storage path resolution that usesos.homedir()is insrc/storage/repositoryStorage.ts, not here.🔎 Proposed fix
import * as vscode from 'vscode'; import * as path from 'path'; -import * as os from 'os'; import { ConfigManager } from './configManager';src/ui/promptTreeProvider.ts (1)
497-533: Consider sanitizing repository identifier for filesystem safety.The
getRepositoryIdentifiermethod extracts parts of URLs but doesn't sanitize the result for filesystem compatibility. Repository names could contain characters that are invalid in filenames on certain platforms (e.g.,<,>,:,",|,?,*on Windows).Suggested enhancement
+ private sanitizeForFilesystem(name: string): string { + // Remove or replace characters invalid in Windows filenames + return name.replace(/[<>:"|?*]/g, '-').replace(/\.+$/, ''); + } + private getRepositoryIdentifier(repositoryUrl: string): string { try { // ... existing logic ... - return pathParts[pathParts.length - 1] || 'repo'; + const identifier = pathParts[pathParts.length - 1] || 'repo'; + return this.sanitizeForFilesystem(identifier); } catch (error) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/copilot-instructions.mdCHANGELOG.mdREADME.mdmedia/main.csspackage.jsonsrc/configManager.tssrc/constant.tssrc/extension.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptCommands.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/extension.ts (1)
src/ui/promptDetailsWebview.ts (1)
PromptDetailsWebviewProvider(9-544)
src/syncManager.ts (3)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-94)src/utils/gitProvider.ts (1)
GitTreeItem(3-9)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
src/ui/promptCommands.ts (1)
src/ui/promptTreeProvider.ts (2)
PromptTreeItem(23-70)PromptInfo(10-21)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptTreeProvider.ts (2)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-94)encodeRepositorySlug(13-21)src/utils/logger.ts (1)
error(68-77)
src/ui/promptDetailsWebview.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-94)encodeRepositorySlug(13-21)
🪛 LanguageTool
CHANGELOG.md
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nating unnecessary counting behavior. - Fixed issue where prompts that were active be...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... broken state after "Deactivate All". - Fixed prompt details view not refreshing when...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...icked while viewing an active prompt. - Fixed duplicate repositories persisting in co...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... and updating settings when detected. - Fixed 'All Sources' dropdown preventing users...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (31)
media/main.css (1)
72-81: LGTM!Clean addition of the new
.type-agentsbadge class while maintaining legacy.type-chatmodesupport. The identical styling ensures visual consistency during the transition from chatmodes to agents terminology.src/constant.ts (1)
1-6: LGTM!Clean migration from
chatmodes/toagents/with well-named legacy path constants for backward compatibility. The comment clearly documents the dual-directory support.src/extension.ts (1)
43-43: LGTM!Correctly passing the extension context to
PromptDetailsWebviewProvider, enabling repository-storage-aware path resolution for inactive prompts on Windows.README.md (2)
84-100: LGTM!Excellent documentation of Windows symlink behavior with clear, actionable instructions for users who want to enable symlinks. The note that file copy mode works fine for most users sets appropriate expectations.
154-158: LGTM!Clear documentation of the new
agents/directory mapping with legacychatmodes/support, matching the code changes insrc/constant.ts.src/configManager.ts (2)
177-230: LGTM!The fallback implementation correctly handles all major platforms with appropriate path structures matching VS Code's standard data locations.
114-175: Profile-aware prompts directory resolution looks good.The logic correctly handles both default and named VS Code profiles by parsing the
globalStorageUripath. The fallback togetFallbackPromptsDirectory()on parsing failures ensures robustness.One minor observation: the path parsing using
split(path.sep)andindexOfworks for standard VS Code installations but could behave unexpectedly if a user's home directory contains 'User' or 'profiles' in the path. This is an edge case but worth noting..github/copilot-instructions.md (2)
13-14: Documentation updates accurately reflect the new architecture.The updated sync description properly documents the new agents/ directory support, legacy chatmodes/ compatibility, repository storage, and Windows symlink fallback behavior.
28-30: Cross-platform conventions are well-documented.The new conventions section correctly captures the key patterns: unique workspace naming for cross-repo conflicts, allowed file types, and the Windows symlink fallback to file copy behavior.
CHANGELOG.md (1)
10-21: Changelog entries comprehensively document the fixes.The entries accurately describe the Windows path handling, bulk operations improvements, duplicate repository cleanup, and sentinel value pattern for the dropdown. The static analysis hints about repetitive "Fixed" can be safely ignored as this follows standard changelog conventions.
src/ui/promptDetailsWebview.ts (2)
116-148: Repository storage path resolution for inactive prompts is well-implemented.The logic correctly prioritizes repository storage for inactive prompts with a repository URL, falls back to workspace path, and includes helpful debug logging for troubleshooting Windows path issues.
18-26: Constructor change to accept optional context is appropriate.The optional
context?: vscode.ExtensionContextparameter maintains backward compatibility while enabling the new repository storage path resolution.src/ui/promptCardsWebview.ts (3)
94-130: Cross-platform path normalization is correctly implemented.The path normalization using
replace(/\\/g, '/')for both the incoming path and stored prompt paths ensures Windows backslash paths match correctly. The enhanced logging will help diagnose any remaining path issues.
703-714: Sentinel value pattern for source filtering is a sound approach.Using
'__none__'as a sentinel to distinguish between "all sources enabled" (empty set) and "no sources selected" (set contains sentinel) correctly solves the dropdown behavior issue described in the PR.
985-988: Windows path escaping for inline JavaScript is correct.Escaping backslashes with
replace(/\\\\/g, '\\\\\\\\')ensures Windows paths likeC:\Users\...are properly escaped when embedded in JavaScript string literals within the HTML template.src/syncManager.ts (6)
794-842: Windows symlink fallback implementation is robust.The
createPromptSymlinkmethod correctly:
- Attempts symlink creation first
- Falls back to file copy on Windows when
EPERMerror occurs (no admin/dev mode)- Logs the operation outcome clearly
- Verifies the target was created
This is the key fix for Windows compatibility.
711-745: Unique workspace naming for cross-repo conflicts is well-designed.The
getUniqueWorkspaceNamemethod correctly checks if a filename exists across multiple repositories and generates a unique name by appending@{repo-identifier}before the extension. This prevents name collisions when activating prompts from different repositories.
1186-1219: Windows file copy handling in cleanup is correctly implemented.The
cleanupOrphanedPromptsmethod properly handles the Windows scenario where active prompts may be file copies instead of symlinks. It checks the tree provider for active status before removing files on Windows, preventing accidental deletion of valid active prompts.
848-871: Symlink removal correctly handles Windows file copies.The
removePromptSymlinkmethod now properly removes both symlinks (Unix/Windows with admin) and regular file copies (Windows fallback), ensuring consistent cleanup behavior across platforms.
162-165: Legacy chatmode path support ensures backward compatibility.Adding
REPO_SYNC_CHAT_MODE_LEGACY_PATHandREPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATHto the allowed paths ensures existing repositories using the oldchatmodes/orchatmode/directories continue to work after the rename toagents/.
1071-1121:activatePromptsignature change returns workspace name.The method now returns
Promise<string>(the workspace filename used) instead ofPromise<void>. This enables callers to track the actual filename used, especially important when unique naming is applied for cross-repo conflicts.All callers properly handle the new return value: src/syncManager.ts:900, src/ui/promptCommands.ts:118, and src/ui/promptCommands.ts:238 each capture and use the returned workspace name for further processing.
src/ui/promptTreeProvider.ts (5)
4-8: LGTM - Imports for multi-repo and cross-platform support.The new imports for
osand repository storage utilities (decodeRepositorySlug,encodeRepositorySlug,getRepositoryStorageDirectory) are correctly added to support the new multi-repository storage architecture.
10-21: LGTM - PromptInfo interface extended for multi-repo support.The interface additions (
workspaceNamefield) and type rename fromchatmodetoagentsalign with the PR objectives. The comments provide clear documentation for each field's purpose.
631-644: LGTM - Type determination with legacy support.The
determinePromptTypemethod correctly handles the rename from 'chatmode' to 'agents' while maintaining backward compatibility with legacy naming patterns (chatmode,chat-mode).
748-779: LGTM - Cross-platform path handling.The
getRepositoryPathandextractRepositoryUrlFromPathmethods properly normalize path separators for cross-platform compatibility, which addresses the Windows path handling issues mentioned in the PR objectives.
440-477: Remove "race condition" terminology - this is a design consistency issue, not a functional bug.The in-memory check in
calculateWorkspaceNameis incomplete during iterative loading (the current prompt hasn't been added tothis.promptsyet), but this is mitigated by the filesystem fallback check (lines 449-476) which correctly scans all repositories. UnlikeSyncManager.getUniqueWorkspaceName, which queries static configuration, this method relies on in-memory state as the primary check. However, the actual result is correct because the filesystem scan happens synchronously before returning, so there's no functional issue. This is a design consistency concern rather than a race condition.Likely an incorrect or invalid review comment.
src/ui/promptCommands.ts (5)
217-219: Redundant null check for syncManager.The
syncManageris already checked at line 197, and TypeScript should narrow the type. This inner check is redundant unlesssyncManagercould be set to undefined between the outer check and the callback execution (which shouldn't happen in synchronous setup).However, keeping it as a defensive check is also acceptable given the async nature of the callback.
195-268: LGTM - Bulk activation with progress feedback.The
selectAllimplementation correctly useswithProgressfor user feedback, handles errors gracefully by collecting them and showing consolidated messages, and refreshes the tree view after completion. The direct mutation of prompt objects (lines 239-240) is acceptable sincetreeProvider.refresh()is called afterward.
270-337: LGTM - Bulk deactivation mirrors selectAll pattern.The
deselectAllimplementation follows the same robust pattern asselectAllwith proper progress feedback, error collection, and tree refresh. UsingworkspaceName || namefor deactivation (line 307) correctly handles the workspace naming logic.
339-370: LGTM - Edit and view prompt handlers.The
editPromptandviewPrompthandlers correctly extract prompt info, use appropriate VS Code APIs, and have proper error handling with user-friendly error messages.
400-436: LGTM - Duplicate prompt with collision handling.The
duplicatePromptmethod properly handles filename collisions by incrementing a counter until a unique name is found. The pattern of_copy,_copy1,_copy2... is intuitive for users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
16-22: Consider varying "Fixed" language to improve readability.Lines 16–22 all begin with "Fixed," creating repetitive phrasing that impacts readability. Since static analysis flagged this, consider rewording a few entries to add variety while preserving clarity.
🔎 Example refactor for varied phrasing
- Fixed "Activate All" to properly create symlinks for all prompts instead of just updating state. - Fixed "Deactivate All" to efficiently remove all symlinks directly instead of looping through individual toggles, eliminating unnecessary counting behavior. - Fixed issue where prompts that were active before "Activate All" would be in a broken state after "Deactivate All". - Fixed prompt details view not refreshing when "Deactivate All" is clicked while viewing an active prompt. - Fixed duplicate repositories persisting in configuration by automatically removing them and updating settings when detected. - Fixed 'All Sources' dropdown preventing users from deselecting the only repository source by implementing sentinel value pattern to distinguish between "all sources" and "no sources selected" states. - Fixed Activity Bar icon not displaying by converting SVG to monochrome format with currentColor support for proper theme adaptation. + "Activate All" now properly creates symlinks for all prompts instead of just updating state. + "Deactivate All" now efficiently removes all symlinks directly instead of looping through individual toggles, eliminating unnecessary counting behavior. + Resolved issue where prompts that were active before "Activate All" would be in a broken state after "Deactivate All". + Prompt details view now refreshes when "Deactivate All" is clicked while viewing an active prompt. + Duplicate repositories no longer persist in configuration—they are automatically removed and settings updated when detected. + 'All Sources' dropdown now allows deselecting the only repository source by implementing sentinel value pattern to distinguish between "all sources" and "no sources selected" states. + Activity Bar icon now displays correctly after converting SVG to monochrome format with currentColor support for proper theme adaptation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/promptitude-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nating unnecessary counting behavior. - Fixed issue where prompts that were active be...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... broken state after "Deactivate All". - Fixed prompt details view not refreshing when...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...icked while viewing an active prompt. - Fixed duplicate repositories persisting in co...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... and updating settings when detected. - Fixed 'All Sources' dropdown preventing users...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~22-~22: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...es" and "no sources selected" states. - Fixed Activity Bar icon not displaying by con...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/ui/promptTreeProvider.ts (2)
587-601: Dead code block has no effect on execution.This code block logs a debug message but never modifies
isActiveFileCopyorrepositoryUrl. The comment suggests it should check repository storage but the implementation only logs. This was flagged in a previous review.🔎 Proposed fix - remove dead code
- // Additional check: if no repository match found but file exists in workspace prompts directory - // it might be a copied file that was just activated - if (!isActiveFileCopy) { - const promptsDir = this.config.getPromptsDirectory(); - const normalizedPromptsDir = promptsDir.replace(/\\/g, '/'); - const normalizedFilePath = filePath.replace(/\\/g, '/'); - - // If the file is in the prompts directory, check all repo storage for a matching file - if (normalizedFilePath.startsWith(normalizedPromptsDir)) { - this.logger.debug(`Windows: File ${fileName} found in workspace prompts directory, checking against repository storage`); - // Note: This prompt is in the workspace directory - // It will be marked as active if we can find it in any repository storage - // The repository URL will remain undefined if not found - } - }
276-278: Minor formatting issue - missing line break before comment.Line 276 has a comment concatenated to the log statement without proper spacing.
🔎 Proposed fix
- this.logger.debug(`Loaded ${workspaceCount} active prompts from workspace`); // Load prompts from repository storage (these are all available prompts) + this.logger.debug(`Loaded ${workspaceCount} active prompts from workspace`); + // Load prompts from repository storage (these are all available prompts)src/syncManager.ts (1)
1122-1139: Remove unuseddeterminePromptTypemethod.This method is not called anywhere in this file. A duplicate implementation exists in
src/ui/promptTreeProvider.ts(lines 636-649) which is the one being used. This was flagged in a previous review.🔎 Proposed fix
- /** - * Determine prompt type from filename - */ - private determinePromptType(fileName: string): 'agents' | 'instructions' | 'prompts' { - const lowerName = fileName.toLowerCase(); - - // Support both 'agents' and legacy 'chatmode' naming - if (lowerName.includes('agent') || lowerName.includes('chatmode') || lowerName.includes('chat-mode')) { - return 'agents'; - } - - if (lowerName.includes('instruction') || lowerName.includes('guide')) { - return 'instructions'; - } - - return 'prompts'; - } -src/ui/promptCardsWebview.ts (1)
407-409: Missing CSS variable--agents-colorstill needs to be defined.The CSS references
var(--agents-color)but only--chatmode-coloris defined in the:rootblock (line 185). This will cause the border color to fall back to the initial value.🔎 Proposed fix
Add the missing variable in the
:rootsection around line 185::root { /* ... existing variables ... */ /* Category colors */ - --chatmode-color: #4CAF50; + --chatmode-color: #4CAF50; + --agents-color: #4CAF50; --instructions-color: #2196F3; --prompts-color: #FF9800; }src/configManager.ts (1)
62-67: Error handler type issue persists.The
.then()rejection handler receives anunknowntype, butthis.logger.errorexpects anErrorobject per the logger signature. This was flagged in a previous review.🔎 Proposed fix
}, (error) => { - this.logger.error('Failed to update repositories configuration', error); + this.logger.error('Failed to update repositories configuration', error instanceof Error ? error : undefined); vscode.window.showWarningMessage( `Found ${repository.length - uniqueArray.length} duplicate repository URL(s) but failed to update configuration. Please remove duplicates manually.` ); });
🧹 Nitpick comments (2)
src/ui/promptCardsWebview.ts (1)
1073-1078: Consider removing or gating console.log statements in production webview.These
console.logcalls in the webview script will appear in the webview's developer console. While harmless, they add noise for end users who might open dev tools. Consider removing them or gating behind a debug flag if persistent logging is needed.src/ui/promptDetailsWebview.ts (1)
111-111: Consider movingrequire('fs')to module-level import.Using
require('fs')inline works but is inconsistent with the rest of the codebase that uses ES module imports. This is a minor style concern.🔎 Proposed fix
Add to the top of the file:
import * as fs from 'fs';Then replace
const fs = require('fs');with direct usage of the importedfsmodule.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/configManager.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/repositoryStorage.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptDetailsWebview.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/syncManager.ts (3)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/utils/gitProvider.ts (1)
GitTreeItem(3-9)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
src/ui/promptTreeProvider.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
🔇 Additional comments (15)
src/ui/promptCardsWebview.ts (3)
94-130: Path normalization and prompt toggle flow look correct.The path normalization using
.replace(/\\/g, '/')for cross-platform comparison is appropriate. The flow correctly:
- Normalizes both the incoming path and stored paths
- Searches for the matching prompt
- Executes the toggle command and refreshes the UI
- Provides user feedback when prompt is not found
The error handling with webview refresh ensures UI stays consistent even on failure.
703-713: Sentinel-based filtering correctly handles edge case.The
'__none__'sentinel value pattern properly distinguishes between "all sources enabled" (empty set) and "no sources selected" (set contains sentinel). This fixes the dropdown filtering bug mentioned in PR objectives.
985-996: Path escaping for Windows compatibility is correct.The double escaping of backslashes (
\\\\) and single quotes (\\') ensures paths render correctly in inline JavaScript strings. This aligns with the approach in other files and addresses Windows path handling issues.src/configManager.ts (2)
113-174: Profile-aware prompts directory resolution is well-structured.The logic correctly handles:
- Custom path override
- Profile-based paths via
globalStorageUriparsing- Graceful fallback when parsing fails
The path construction using
path.sepensures cross-platform compatibility, and the debug logging aids troubleshooting.
176-229: Platform-specific fallback paths are correct.The fallback directory paths for Windows, macOS, and Linux match VS Code's standard user data locations. The error handling with a descriptive exception message is appropriate.
src/ui/promptDetailsWebview.ts (2)
16-25: Constructor extension for context parameter is appropriate.Storing the optional
contextenables proper repository storage path resolution viagetRepositoryStorageDirectory(this.context).
110-159: Repository storage path resolution for inactive prompts is correct.The logic correctly prioritizes:
- Repository storage for inactive prompts with
repositoryUrl- Workspace path if the file exists there
- Original path as fallback
The debug logging and directory listing on failure will help diagnose path issues on Windows.
src/ui/promptTreeProvider.ts (4)
10-19: PromptInfo interface updates are appropriate.The type change from
'chatmode'to'agents'and addition ofworkspaceNamefield align with the PR's category renaming and unique workspace naming features.
409-418: Content comparison for Windows file copies addresses false positive concern.The previous review flagged that Windows active detection could produce false positives. This implementation now compares file content (
workspaceContent === content) to verify the file is actually from the repository, not just a file with the same name.
636-649: Legacy chatmode support in determinePromptType is good.Supporting both 'agents' and legacy 'chatmode'/'chat-mode' naming ensures backward compatibility with existing prompt files while transitioning to the new naming.
764-784: Cross-platform path normalization in extractRepositoryUrlFromPath is correct.Normalizing path separators to forward slashes before parsing ensures consistent behavior across Windows and Unix systems.
src/syncManager.ts (4)
789-841: Windows symlink fallback implementation is robust.The
createPromptSymlinkfunction correctly:
- Attempts symlink creation first
- Falls back to file copy on Windows with EPERM error
- Verifies the target file was created
- Provides clear logging for debugging
This addresses the core Windows compatibility issue in the PR.
1067-1120: activatePrompt now returns workspace name correctly.The function signature change to
Promise<string>enables callers to track the actual workspace filename used (which may differ from the original filename when disambiguation is needed). The verification step ensures the file was actually created.
1206-1218: Windows-aware orphan cleanup correctly preserves active file copies.The cleanup logic now checks if a file is an active prompt on Windows before removing it, preventing accidental deletion of valid file copies (the symlink fallback mechanism).
710-787: Acknowledge duplicated workspace naming logic.The
getUniqueWorkspaceNameandgetRepositoryIdentifiermethods duplicate logic frompromptTreeProvider.ts. The author acknowledged this duplication and deferred consolidation to a future PR, which is acceptable for keeping this bug-fix PR focused.Based on past review comments, this is a known deferral.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/syncManager.ts (2)
671-683: Migration fallback uses Unix-onlycpcommand, will fail on Windows.The
spawnSync('cp', ['-R', ...])fallback at line 674 uses a Unix command that doesn't exist natively on Windows. Since this PR explicitly targets Windows compatibility, this migration path will silently fail on Windows systems.🔎 Proposed fix using cross-platform approach
} catch (error) { // If rename fails, try copy and delete this.logger.warn('Could not move repository storage, attempting copy...'); - const ncp = require('child_process').spawnSync('cp', ['-R', oldRepoStorage, newRepoStorage]); - - if (ncp.status === 0) { - // Delete old directory after successful copy - await fs.rm(oldRepoStorage, { recursive: true, force: true }); - this.logger.info('Successfully copied and removed old repository storage'); - } else { - this.logger.error('Failed to migrate repository storage'); + try { + await fs.cp(oldRepoStorage, newRepoStorage, { recursive: true }); + // Delete old directory after successful copy + await fs.rm(oldRepoStorage, { recursive: true, force: true }); + this.logger.info('Successfully copied and removed old repository storage'); + } catch (copyError) { + this.logger.error('Failed to migrate repository storage', copyError instanceof Error ? copyError : undefined); } }Note:
fs.cprequires Node.js 16.7.0+ (available in VS Code's bundled Node). Alternatively, use a recursive copy utility.
1010-1016: URL decoding doesn't match the Base64 encoding used byencodeRepositorySlug.Lines 1012-1015 use simple string replacement (underscore-to-slash), but
encodeRepositorySluginsrc/storage/repositoryStorage.tsuses Base64 URL encoding. The codebase already provides adecodeRepositorySlugfunction that properly decodes Base64 URLs. Use that function instead of the manual replacement to correctly extract repository URLs from symlink targets.src/ui/promptCardsWebview.ts (1)
1014-1021: Update getTypeIcon to use 'agents' instead of 'chatmode'.The function still has a case for
'chatmode'(line 1016) but the rest of the file has migrated to using'agents'. This case is now unreachable dead code.🔎 Proposed fix
function getTypeIcon(type) { switch(type) { - case 'chatmode': return '💬'; + case 'agents': return '🤖'; case 'instructions': return '📖'; case 'prompts': return '⚡'; default: return '📄'; } }Note: Using 🤖 to match the icon used in the filter button (line 813).
♻️ Duplicate comments (3)
src/configManager.ts (2)
50-68: Duplicate removal logic looks correct; error handling improved.The fix at line 63 properly handles the
unknownerror type by checkingerror instanceof Error. The async configuration update withoutawaitwas flagged previously - since the developer deferred this, I'll note that this is a fire-and-forget pattern that could lose updates on extension reload, but the user-facing warning message provides adequate fallback guidance.
136-149: Profile path parsing uses string matching on path segments.As noted in a previous review, this approach could fail if a user has 'profiles' in their username or custom path. The developer has acknowledged this and deferred to a future PR.
src/syncManager.ts (1)
710-744: Unique workspace naming logic is sound.The approach of appending
@org-repoto disambiguate duplicate filenames across repositories is reasonable. The async file existence checks for each repository could be slow with many repositories, but this is unlikely to be a practical issue.As noted in a previous review, this logic duplicates
calculateWorkspaceNameinpromptTreeProvider.ts- the developer has deferred this DRY refactoring.
🧹 Nitpick comments (7)
src/syncManager.ts (1)
798-798: Redundant inlinerequire('path')whenpathis already imported.Line 798 uses
require('path').dirname(targetPath)butpathis already imported at line 2. Use the existing import for consistency.🔎 Proposed fix
- const targetDir = require('path').dirname(targetPath); + const targetDir = path.dirname(targetPath);src/ui/promptCardsWebview.ts (6)
45-45: Consider sanitizing message data in logs.The debug log uses
JSON.stringify(message)which could expose sensitive data such as file paths or configuration details. While this is valuable for debugging, consider whether debug logs should be conditional or if certain fields should be redacted in production environments.
94-130: Approve path normalization; consider verbose error logging.The path normalization logic for cross-platform compatibility is correct and necessary. The extensive logging throughout is helpful for debugging.
However, line 122 logs all available paths when a prompt is not found, which could be very verbose if there are many prompts. Consider limiting this to debug level or truncating the output.
🔎 Optional: limit verbose path logging
- this.logger.debug(`[WebView] Available paths: ${allPrompts.map(p => p.path).join(', ')}`); + this.logger.debug(`[WebView] Available paths (showing first 10): ${allPrompts.slice(0, 10).map(p => p.path).join(', ')}${allPrompts.length > 10 ? '...' : ''}`);
756-757: Sentinel logic is correct.The sentinel value checks throughout the dropdown rendering are logically correct. The pattern consistently distinguishes between "all sources" (empty set, no sentinel) and "no sources" (sentinel present).
The double negative
!hasNoneSentinelon lines 776 and 791 works correctly but could be slightly more readable as a positive check.💡 Optional: improve readability
Consider introducing a helper variable:
// Check if sentinel value is present (no sources selected) const hasNoneSentinel = enabledSources.has('__none__'); +const allSourcesEnabled = enabledSources.size === 0 && !hasNoneSentinel; // ... -const isChecked = (enabledSources.size === 0 && !hasNoneSentinel) || enabledSources.has(url); +const isChecked = allSourcesEnabled || enabledSources.has(url);Also applies to: 776-776, 791-791, 803-803, 805-805
887-928: Approve sentinel-based source toggle logic.The source toggle logic correctly handles the edge case of deselecting the only available source using the sentinel pattern. The implementation covers all state transitions:
- All sources enabled → deselect one → specific sources enabled (or sentinel if only one)
- Some sources enabled → deselect last → sentinel added
- Sentinel present → select source → sentinel removed
- All individual sources selected → set cleared to "all enabled" state
This is complex state management that would benefit from unit tests to ensure correctness across all edge cases.
986-987: Consider more robust path escaping.The current escaping approach handles backslashes and single quotes, which covers the most common Windows path cases. However, as noted in a previous review, paths can contain other special characters that may need escaping.
🔎 More robust escaping approach
Consider using
JSON.stringifywhich handles all special characters:- // Escape characters in paths for JavaScript single-quoted strings (Windows compatibility) - const escapedPath = String(prompt.path).replace(/\\/g, '\\\\').replace(/'/g, '\\\''); + // Escape path for safe use in HTML onclick attributes + const escapedPath = JSON.stringify(prompt.path).slice(1, -1); // Remove surrounding quotesThen use it in double-quoted strings:
- <div class="prompt-card ${prompt.type} ${prompt.active ? 'active' : ''}" onclick="viewPrompt('${escapedPath}')"> + <div class="prompt-card ${prompt.type} ${prompt.active ? 'active' : ''}" onclick="viewPrompt('${escapedPath}')">Alternatively, consider using data attributes to avoid inline event handlers entirely:
<div class="prompt-card" data-path="${escapeHtml(prompt.path)}" onclick="handleCardClick(event)">function handleCardClick(event) { const path = event.currentTarget.getAttribute('data-path'); viewPrompt(path); }
1074-1074: Consider removing or conditionalizing debug logs.The console.log statements are useful for debugging but will appear in production. Consider either:
- Removing them after debugging is complete
- Wrapping them in a debug flag check
- Keeping them if they provide value for troubleshooting user issues
Given the "[Promptitude WebView]" prefix and the focused nature of the logging, keeping them is reasonable if they help with user support.
Also applies to: 1079-1079
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.jsonsrc/configManager.tssrc/syncManager.tssrc/ui/promptCardsWebview.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/syncManager.ts (3)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/utils/gitProvider.ts (1)
GitTreeItem(3-9)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
🔇 Additional comments (15)
src/configManager.ts (2)
113-174: Profile-aware prompts directory resolution looks solid.The logic correctly handles:
- Custom path override (line 121-124)
- Profile vs default path detection (line 136)
- Fallback on parse failure (lines 147-148, 160-161)
One minor observation: the
Userfolder name is case-sensitive in the path matching (line 153), which should work correctly on all platforms since VS Code uses consistent casing internally.
176-229: Fallback path resolution handles all major platforms.The platform-specific paths align with standard VS Code data locations. The catch block at line 226 throws a new error, which is appropriate since this is a critical failure.
src/syncManager.ts (9)
8-13: Imports look appropriate for the new functionality.The new imports for
encodeRepositorySlug,getRepositoryStorageDirectory, and legacy path constants support the refactored storage and sync logic.
807-833: Windows symlink fallback with EPERM handling looks correct.The fallback to file copy when symlinks fail on Windows (EPERM error) is a reasonable approach. The error is properly re-thrown for non-EPERM cases and Unix systems.
1067-1120: Activation logic with unique workspace naming looks correct.The updated method properly:
- Validates repository URL requirement
- Generates unique workspace names for disambiguation
- Creates symlinks/copies via the Windows-compatible helper
- Verifies target creation before returning
The return type change from
Promise<void>toPromise<string>enables callers to track the actual workspace filename used.
160-164: Legacy chatmodes path support added correctly.Adding support for
chatmodes/andchatmode/directories maintains backwards compatibility with older repository structures. The debug logging helps with troubleshooting sync issues.
500-500: Terminology updated from ChatMode to Agents.The status display now uses "Agents" terminology consistently, aligning with the updated naming convention across the extension.
Also applies to: 575-575
858-861: Windows file copy removal handled correctly.The method now properly removes both symlinks and regular file copies (the Windows fallback), ensuring consistent cleanup behavior across platforms.
899-906: Symlink recreation tracks workspace name changes.The logic correctly updates
prompt.workspaceNamewhen the actual workspace filename differs from the original, handling disambiguation cases properly.
1225-1235: Dispose method looks clean.The cleanup logic properly clears the timer on disposal, and the
determinePromptTypedead code method has been removed from the file.
767-773: Legacy Azure DevOps URLs won't parse correctly in this function.The code explicitly handles
dev.azure.com/org/project/_git/repobut not legacyorg.visualstudio.com/project/_git/repoformat. For legacy URLs, the fallback logic will extract the last two path segments (e.g.,_git-repo), producing less descriptive identifiers than the modern format.src/ui/promptCardsWebview.ts (4)
63-65: LGTM!The default case for unknown commands is a good defensive practice that helps identify messaging issues during development and debugging.
185-185: LGTM! Addresses previous review comment.The
--agents-colorCSS variable is now defined, resolving the issue flagged in the previous review wherevar(--agents-color)was referenced but undefined.
408-410: LGTM!The agents category styling is consistent with other categories and correctly uses the
--agents-colorvariable.Also applies to: 466-468
704-715: LGTM! Sentinel pattern resolves the deselection issue.The sentinel value
__none__correctly distinguishes between "all sources enabled" (empty set) and "no sources selected" (set containing__none__), fixing the dropdown filtering issue described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/promptCardsWebview.ts (1)
1014-1021: Dead 'chatmode' case has been removed, but icon mismatch exists.The 'chatmode' case mentioned in past review comments has been removed. However, the
agentscase now returns'💬'(speech bubble) while the category rendering uses'🤖'(robot) for Agents (line 957). This creates an inconsistent user experience.🔎 Proposed fix
function getTypeIcon(type) { switch(type) { - case 'agents': return '💬'; + case 'agents': return '🤖'; case 'instructions': return '📖'; case 'prompts': return '⚡'; default: return '📄'; } }src/syncManager.ts (1)
1000-1023: FixextractRepositoryUrlFromTargetPathto use the correct base64url decoding logic.The method uses
.replace(/_/g, '/').replace(/^/, 'https://')to decode repository URLs, but the codebase encodes repository slugs using base64url encoding (seeencodeRepositorySluganddecodeRepositorySluginsrc/storage/repositoryStorage.ts). The underscore replacement logic is incompatible with base64url. UsedecodeRepositorySlug(encodedRepoUrl)at line 1012 instead.
🧹 Nitpick comments (1)
src/ui/promptCardsWebview.ts (1)
886-900: Consider extracting sentinel constant to avoid magic string duplication.The sentinel value
'__none__'appears in multiple places (lines 705, 757, 892, 909, 914). Extract it to a constant at the top of the script to improve maintainability and prevent typos.🔎 Suggested refactor
Add at the beginning of the script block (around line 650):
const NO_SOURCES_SENTINEL = '__none__';Then replace all occurrences of
'__none__'withNO_SOURCES_SENTINEL.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/syncManager.tssrc/ui/promptCardsWebview.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/syncManager.ts (4)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/utils/logger.ts (1)
error(68-77)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)src/configManager.ts (1)
repositories(44-71)
🔇 Additional comments (12)
src/ui/promptCardsWebview.ts (5)
45-65: Enhanced logging and default case handling look good.The added
[WebView]prefix improves log traceability, and the default case (lines 63-64) properly handles unknown commands with a warning instead of silently ignoring them.
94-130: Path normalization for cross-platform compatibility is correctly implemented.The normalization of backslashes to forward slashes (line 99, 106) ensures Windows paths match correctly during prompt lookup. The error handling and UI refresh on failure (lines 128-129) maintain consistency.
185-189: CSS variable--agents-colornow correctly defined.The previously missing
--agents-colorvariable is now defined alongside--chatmode-color. Note that--chatmode-color(line 186) appears to be retained for backward compatibility but could be removed if no longer referenced elsewhere.
704-714: Sentinel value pattern for "no sources selected" is a clean solution.Using
'__none__'as a sentinel distinguishes between "all sources enabled" (empty set) and "no sources selected" (set contains sentinel). This correctly fixes the dropdown filtering bug mentioned in the PR objectives.
986-996: Path escaping handles backslashes and single quotes for Windows compatibility.The escaping logic at lines 986-987 correctly escapes both backslashes and single quotes for safe embedding in JavaScript string literals. This addresses the Windows path handling issue mentioned in the PR objectives.
src/syncManager.ts (7)
160-164: Legacy chatmode path support correctly implemented.The addition of
REPO_SYNC_CHAT_MODE_LEGACY_PATHandREPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATHensures backward compatibility with existing repositories usingchatmodes/orchatmode/directories.
793-841: Windows symlink fallback to file copy is well-implemented with proper error handling.The logic correctly:
- Attempts symlink creation first (line 812)
- Falls back to file copy only on Windows with EPERM error (lines 818-827)
- Re-throws errors on Unix or other Windows errors (lines 828-832)
- Verifies target creation (lines 836-840)
The past review comment about enriching error messages when both approaches fail was acknowledged and deferred.
854-862: Remove symlink correctly handles Windows file copy fallback.The method now properly removes both symlinks (line 856-857) and Windows file copies (lines 858-861), maintaining parity with the creation logic.
950-963: Path normalization infixBrokenSymlinksimproves cross-platform symlink repair.The normalization of target paths (line 952-953) before comparison ensures symlinks created on one platform can be properly analyzed on another.
1070-1114:activatePromptsignature change returns workspace name for caller synchronization.The method now returns
Promise<string>(the workspace filename) instead ofPromise<void>. This enables callers to track the actual filename used, especially important when disambiguation adds repository identifiers.The error thrown when
repositoryUrlis missing (lines 1071-1075) is appropriate since repository context is now required for the unique naming logic.
1193-1205: Windows active file check now correctly handles disambiguated filenames.The fix at lines 1198-1200 addresses the past review comment by checking both
p.nameandp.workspaceNameagainstentry.name. This ensures files with repository suffixes (e.g.,prompt@org-repo.md) are correctly identified as active and not deleted.
751-787:getRepositoryIdentifierspecifically handles modern Azure DevOps format—other URL types use generic fallback.The Azure DevOps parsing at lines 768–772 is intentionally limited to the modern
dev.azure.com/{org}/{project}/_git/{repo}format. Legacy visualstudio.com URLs, SSH URLs (e.g.,git@ssh.dev.azure.com:v3/{org}/{project}/{repo}), and on-premises Azure DevOps Server instances will fall through to the generic path-segment handler, which extracts the last two path components. Verify this behavior is acceptable for your repository URL sources.
mohamedsedkiyouzbechi
force-pushed
the
msyo/fix-promptitude-ui
branch
from
515994c to
873eee3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
package.json (1)
79-83: VerifysyncInstructionsdefault consistency with configManager.ts.The default here is
false. Confirm thatsrc/configManager.tsline 102 also usesfalseas the fallback to ensure consistency. Based on past review comments, this was previously flagged as a mismatch.#!/bin/bash # Verify syncInstructions default value in configManager.ts grep -n "syncInstructions" src/configManager.ts | head -5src/syncManager.ts (2)
674-682: Replace Unixcpcommand with cross-platform alternative.The
spawnSync('cp', ...)command at line 674 fails on Windows sincecpis not a native command. Use Node.jsfs.cp()(available since Node 16.7.0) for cross-platform compatibility.🔎 Proposed fix
} catch (error) { // If rename fails, try copy and delete this.logger.warn('Could not move repository storage, attempting copy...'); - const ncp = require('child_process').spawnSync('cp', ['-R', oldRepoStorage, newRepoStorage]); - - if (ncp.status === 0) { + try { + await fs.cp(oldRepoStorage, newRepoStorage, { recursive: true }); // Delete old directory after successful copy await fs.rm(oldRepoStorage, { recursive: true, force: true }); this.logger.info('Successfully copied and removed old repository storage'); - } else { + } catch (copyError) { this.logger.error('Failed to migrate repository storage'); } }
710-787: Code duplication with promptTreeProvider.ts.The
getUniqueWorkspaceNameandgetRepositoryIdentifiermethods duplicate logic frompromptTreeProvider.ts. This was acknowledged in past reviews and deferred to a future PR. Consider extracting to a shared utility module when time permits.src/ui/promptTreeProvider.ts (2)
587-601: Remove incomplete/dead code block.This code block logs a debug message but never modifies
isActiveFileCopyorrepositoryUrl. The comment states it should "check all repo storage for a matching file" but the implementation only logs. This was flagged in past reviews as dead code.🔎 Proposed fix - remove dead code
- // Additional check: if no repository match found but file exists in workspace prompts directory - // it might be a copied file that was just activated - if (!isActiveFileCopy) { - const promptsDir = this.config.getPromptsDirectory(); - const normalizedPromptsDir = promptsDir.replace(/\\/g, '/'); - const normalizedFilePath = filePath.replace(/\\/g, '/'); - - // If the file is in the prompts directory, check all repo storage for a matching file - if (normalizedFilePath.startsWith(normalizedPromptsDir)) { - this.logger.debug(`Windows: File ${fileName} found in workspace prompts directory, checking against repository storage`); - // Note: This prompt is in the workspace directory - // It will be marked as active if we can find it in any repository storage - // The repository URL will remain undefined if not found - } - }
445-538: Code duplication with SyncManager.The
calculateWorkspaceNameandgetRepositoryIdentifiermethods duplicate logic fromSyncManager. This was acknowledged in past reviews and deferred. Consider extracting to a shared utility (e.g.,src/utils/workspaceNaming.ts) when time permits.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonsrc/configManager.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptDetailsWebview.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/syncManager.ts (2)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (15)
src/storage/repositoryStorage.ts (1)
45-98: LGTM! Well-structured repository storage path resolution.The function properly prioritizes
context.globalStorageUriwhen available and falls back to platform-specific paths. The extension ID is now correctly derived frompackage.jsonas suggested in previous reviews.package.json (1)
5-5: LGTM! Version bump and description update.The version increment to 1.5.2 is appropriate for the bug fixes in this PR, and the updated description for
syncChatmodeaccurately reflects the newagents/directory support alongside legacy paths.src/configManager.ts (1)
113-174: LGTM! Profile-aware prompts directory resolution.The implementation correctly handles both default and named VS Code profiles by parsing the
globalStorageUripath structure. The fallback mechanism ensures the extension works even without context. The fragile path parsing was acknowledged in past reviews and deferred to a future PR.src/ui/promptDetailsWebview.ts (2)
110-159: LGTM! Improved path resolution for inactive prompts.The updated
getActualFilePathcorrectly routes inactive prompts with arepositoryUrlto repository storage, while falling back to workspace paths for active prompts. The debug logging aids troubleshooting.Note: The use of synchronous
fsoperations (require('fs'),readdirSync) was flagged in past reviews and acknowledged to be deferred to a future PR.
16-26: LGTM! Constructor updated for context support.The optional
contextparameter enables reliable repository storage path resolution viagetRepositoryStorageDirectory(this.context).src/ui/promptCardsWebview.ts (4)
704-714: LGTM! Sentinel value pattern for empty source selection.The
'__none__'sentinel value elegantly handles the edge case where the user deselects the only repository source, allowing the UI to distinguish between "all sources" (empty set) and "no sources" (sentinel present).
94-130: LGTM! Cross-platform path normalization for toggle.Normalizing paths with
replace(/\\/g, '/')ensures Windows backslashes don't break the prompt lookup. The enhanced logging aids debugging.
986-997: LGTM! Path escaping for Windows compatibility.Escaping backslashes and single quotes in paths before inserting into inline JavaScript prevents syntax errors on Windows.
1014-1021: Dead 'chatmode' case successfully removed from getTypeIcon().The function contains only the necessary cases: 'agents', 'instructions', 'prompts', and default. The removal is complete and correct.
src/syncManager.ts (3)
793-841: LGTM! Robust symlink creation with Windows fallback.The implementation correctly attempts symlink creation first, then falls back to file copy on Windows when
EPERMis encountered. The verification step ensures the target was created successfully.
1070-1120: LGTM! activatePrompt now returns workspace name.The updated signature
Promise<string>returning the actual workspace filename enables callers to track disambiguated names. Good validation and logging throughout.
1193-1205: LGTM! Cleanup now handles disambiguated filenames.The fix at lines 1198-1200 correctly checks both
p.nameandp.workspaceNameto avoid incorrectly removing active Windows file copies that have been renamed with repository identifiers (e.g.,prompt@org-repo.md). This addresses the past review comment.src/ui/promptTreeProvider.ts (3)
9-20: LGTM! PromptInfo interface updated for cross-repo support.The addition of
workspaceNameenables tracking disambiguated filenames (e.g.,prompt@org-repo.md), and the type union now uses'agents'instead of'chatmode'.
399-423: LGTM! Windows active detection now uses content comparison.The implementation correctly verifies that a Windows file copy matches the repository content before marking it as active, preventing false positives from user-created files with matching names. This addresses the past review concern.
636-649: LGTM! determinePromptType correctly handles legacy naming.The function now returns
'agents'for both newagentpatterns and legacychatmode/chat-modepatterns, ensuring backward compatibility.
mohamedsedkiyouzbechi
force-pushed
the
msyo/fix-promptitude-ui
branch
from
873eee3 to
015530a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/syncManager.ts (1)
674-675: Critical: Unixcpcommand fails on Windows during repository migration.Line 674 uses
child_process.spawnSync('cp', ...)which doesn't exist on Windows. While this fallback only runs iffs.renamefails, the migration will fail on Windows if rename doesn't work. Replace with Node's cross-platformfs.cpSync()orfs.promises.cp()(available since Node 16.7.0, well-supported in VS Code 2024).🔎 Proposed fix
} catch (error) { // If rename fails, try copy and delete this.logger.warn('Could not move repository storage, attempting copy...'); - const ncp = require('child_process').spawnSync('cp', ['-R', oldRepoStorage, newRepoStorage]); - - if (ncp.status === 0) { + try { + // Use cross-platform recursive copy (Node 16.7.0+) + const fs = require('fs'); + fs.cpSync(oldRepoStorage, newRepoStorage, { recursive: true }); // Delete old directory after successful copy await fs.rm(oldRepoStorage, { recursive: true, force: true }); this.logger.info('Successfully copied and removed old repository storage'); - } else { + } catch (copyError) { this.logger.error('Failed to migrate repository storage'); } }src/configManager.ts (1)
63-63: Critical: Stray '+' character causes syntax error.Line 63 has a leading
+character beforethis.logger.error, which is a merge artifact that will cause a compilation failure.🔎 Proposed fix
}, (error) => { -+ this.logger.error('Failed to update repositories configuration', error instanceof Error ? error : undefined); + this.logger.error('Failed to update repositories configuration', error instanceof Error ? error : undefined); vscode.window.showWarningMessage(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonsrc/configManager.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/repositoryStorage.ts
- package.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/ui/promptTreeProvider.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptDetailsWebview.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/syncManager.ts (2)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
🔇 Additional comments (2)
src/ui/promptCardsWebview.ts (2)
986-987: LGTM: Path escaping correctly handles Windows paths in inline JavaScript.Lines 986-987 escape backslashes and single quotes before embedding paths in single-quoted JavaScript strings. This prevents syntax errors and potential injection when paths contain special characters (especially Windows backslashes). The escaped paths are correctly used in onclick handlers on lines 989 and 996.
Also applies to: 989-989, 996-996
704-715: Sentinel pattern logic is correct and properly implemented.The sentinel value
'__none__'correctly distinguishes "no sources selected" from "all sources selected" (empty Set). All state transitions are implemented correctly:
- Sentinel is added when the last source is deselected (line 911)
- Sentinel is removed when any source is selected (line 914)
- Filters to empty array when sentinel is present (lines 706-707)
- Checkbox states properly reflect all three states via
hasNoneSentinelvariable (lines 757, 776, 791)The three states are accurately represented:
enabledSources.size === 0AND no sentinel → all sources enabledenabledSources.size > 0AND no sentinel → specific sources enabled- Sentinel present → no sources selected
Consider adding a consolidated comment in the state management section documenting these three states to aid future maintainers.
mohamedsedkiyouzbechi
force-pushed
the
msyo/fix-promptitude-ui
branch
from
fa8e6c2 to
05dea06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/ui/promptTreeProvider.ts (2)
587-601: Dead/incomplete code block remains.This block logs a debug message but never modifies
isActiveFileCopyorrepositoryUrl. A past review flagged this as dead code and marked it as addressed, but the code still appears to be present. Either remove it or implement the intended functionality.🔎 Remove dead code
- // Additional check: if no repository match found but file exists in workspace prompts directory - // it might be a copied file that was just activated - if (!isActiveFileCopy) { - const promptsDir = this.config.getPromptsDirectory(); - const normalizedPromptsDir = promptsDir.replace(/\\/g, '/'); - const normalizedFilePath = filePath.replace(/\\/g, '/'); - - // If the file is in the prompts directory, check all repo storage for a matching file - if (normalizedFilePath.startsWith(normalizedPromptsDir)) { - this.logger.debug(`Windows: File ${fileName} found in workspace prompts directory, checking against repository storage`); - // Note: This prompt is in the workspace directory - // It will be marked as active if we can find it in any repository storage - // The repository URL will remain undefined if not found - } - }
449-538: Workspace naming logic duplication noted.This duplicates
SyncManager.getUniqueWorkspaceNameandgetRepositoryIdentifier. A past review flagged this DRY violation, and the author deferred extraction to a shared utility module for a future PR. This is acceptable for the current bug-fix scope.src/syncManager.ts (1)
674-682: Unixcpcommand will fail on Windows.The fallback migration logic uses
spawnSync('cp', ...)which only works on Unix systems. On Windows, this command doesn't exist and migration will fail. Use Node.js's cross-platformfs.cp()(available since Node 16.7.0) instead.🔎 Cross-platform fix
} catch (error) { // If rename fails, try copy and delete this.logger.warn('Could not move repository storage, attempting copy...'); - const ncp = require('child_process').spawnSync('cp', ['-R', oldRepoStorage, newRepoStorage]); - - if (ncp.status === 0) { + try { + await fs.cp(oldRepoStorage, newRepoStorage, { recursive: true }); // Delete old directory after successful copy await fs.rm(oldRepoStorage, { recursive: true, force: true }); this.logger.info('Successfully copied and removed old repository storage'); - } else { + } catch (copyError) { this.logger.error('Failed to migrate repository storage'); } }src/configManager.ts (1)
63-63: Critical: Remove stray+character causing syntax error.Line 63 contains a leading
+character that will cause a TypeScript syntax error. This appears to be a merge artifact that was not cleaned up.🔎 Proposed fix
}, (error) => { -+ this.logger.error('Failed to update repositories configuration', error instanceof Error ? error : undefined); + this.logger.error('Failed to update repositories configuration', error instanceof Error ? error : undefined);
🧹 Nitpick comments (1)
src/ui/promptCardsWebview.ts (1)
986-987: Path escaping may be incomplete for edge cases.The current escaping only handles backslashes. On Windows, paths could theoretically contain single quotes (though rare), which would break the JavaScript string literal. A past review suggested also escaping single quotes.
🔎 More robust escaping
// Escape characters in paths for JavaScript single-quoted strings (Windows compatibility) - const escapedPath = prompt.path.replace(/\\\\/g, '\\\\\\\\'); + const escapedPath = prompt.path.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonsrc/configManager.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/ui/promptTreeProvider.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/syncManager.ts (5)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/utils/notifications.ts (1)
NotificationManager(6-165)src/utils/logger.ts (1)
error(68-77)src/utils/gitProvider.ts (1)
GitTreeItem(3-9)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (13)
src/configManager.ts (1)
113-174: Profile-aware prompts directory resolution looks good.The implementation correctly handles both default and named VS Code profiles by parsing
globalStorageUri. The fallback mechanism ensures graceful degradation when context is unavailable.Note: A past review flagged that path parsing using
profilesindex could theoretically match if a user has 'profiles' in their username. The author deferred this to a future PR, which is acceptable given the low likelihood.src/storage/repositoryStorage.ts (2)
45-98: Repository storage directory resolution is well-implemented.The function correctly prioritizes context-based resolution and falls back to platform-specific paths. The structure mirrors VS Code's globalStorage layout appropriately.
52-56: The fallback extension ID'logientnventive.promptitude-extension'is correct and matches thepublisher.namepattern in package.json.src/ui/promptDetailsWebview.ts (2)
110-159: Path resolution for inactive prompts handles Windows correctly.The
getActualFilePathmethod properly resolves paths for inactive prompts by first checking repository storage (using the newgetRepositoryStorageDirectoryhelper), then falling back to workspace path. The cross-platform path handling and detailed debug logging will aid troubleshooting.Note: A past review flagged the use of synchronous
fs.readdirSync(line 139) inside this async context. The author deferred this to a future PR, which is acceptable given it's only used for debug logging.
16-26: Context parameter addition is clean.The optional
contextparameter allows path resolution to use VS Code'sglobalStorageUriwhen available, improving reliability over fallback paths.src/ui/promptCardsWebview.ts (2)
704-714: Sentinel value pattern for source filtering works correctly.The
'__none__'sentinel value distinguishes between "all sources enabled" (empty set) and "no sources selected" (set containing only sentinel). The filtering logic correctly handles all three states: all, none, and specific sources.
185-189: CSS variables properly support agents terminology.The
--agents-colorvariable is now defined alongside the legacy--chatmode-color(which is still used for action button styling). This ensures both old and new CSS selectors work correctly.src/ui/promptTreeProvider.ts (2)
409-423: Windows active detection with content verification addresses false positive concern.The implementation now verifies content matches between workspace file and repository file before marking a prompt as active on Windows. This prevents incorrectly marking user-created files with matching names as "active" prompts.
363-373: Composite key lookup prevents cross-repo collisions.Using both filename and repositoryUrl as a composite key ensures prompts with the same name from different repositories are correctly distinguished. This is essential for the multi-repository prompt management feature.
src/syncManager.ts (4)
793-841: Windows symlink fallback to file copy is well-implemented.The
createPromptSymlinkmethod properly handles Windows EPERM errors by falling back to file copy. The post-creation verification ensures the operation succeeded regardless of which method was used. This addresses the core Windows compatibility issue in the PR.
1070-1120: activatePrompt signature change is properly integrated.The method now returns
Promise<string>(the actual workspace filename used). This is correctly utilized byrecreateActivePromptSymlinksto update prompt metadata. The detailed logging aids troubleshooting activation issues.
1193-1205: Cleanup correctly handles disambiguated filenames on Windows.The check now compares both
p.nameandp.workspaceNameagainst the entry name, preventing accidental deletion of active prompts that were renamed with@org-reposuffix for disambiguation. This addresses a past review concern.
1000-1023: Cross-platform path normalization in URL extraction is correct.Path normalization using forward slashes enables consistent parsing of symlink targets across Windows and Unix. The extraction logic correctly handles both path separator styles.
mohamedsedkiyouzbechi
force-pushed
the
msyo/fix-promptitude-ui
branch
from
fcd033c to
8d1ae7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/storage/repositoryStorage.ts (1)
45-98: Add error handling for package.json require.Line 52 uses
require('../../package.json')without error handling. Whilepackage.jsonshould always exist in a properly installed extension, adding a try-catch provides defense against corruption or unusual installation scenarios and would make debugging easier.🔎 Proposed improvement
// Fallback: use platform-specific globalStorage path - const packageJson = require('../../package.json') as { publisher?: string; name?: string }; - const extensionId = - packageJson?.publisher && packageJson?.name - ? `${packageJson.publisher}.${packageJson.name}` - : 'logientnventive.promptitude-extension'; + let extensionId = 'logientnventive.promptitude-extension'; + try { + const packageJson = require('../../package.json') as { publisher?: string; name?: string }; + if (packageJson?.publisher && packageJson?.name) { + extensionId = `${packageJson.publisher}.${packageJson.name}`; + } + } catch (error) { + logger.warn('Failed to read package.json, using fallback extension ID', error instanceof Error ? error : undefined); + }src/ui/promptCardsWebview.ts (1)
986-999: Consider more robust path escaping for inline event handlers.The current escaping (line 987) only doubles backslashes, which handles typical Windows paths. However, if a path contains single quotes, the inline
onclickstrings (lines 989, 996) could break. While rare, this is possible with user-created paths.🔎 Suggested improvement
Consider using
JSON.stringify()for safer escaping, or usedata-pathattributes with event delegation instead of inlineonclick:// Option 1: JSON.stringify for safer escaping const escapedPath = JSON.stringify(prompt.path).slice(1, -1); // Remove quotes // Option 2: Use data attributes (better practice) // In renderPromptCard: <div class="prompt-card" data-path="${escapeHtml(prompt.path)}" onclick="handleCardClick(event)"> ... <button data-path="${escapeHtml(prompt.path)}" onclick="handleToggle(event)"> // Then in JS: function handleCardClick(event) { const path = event.currentTarget.dataset.path; viewPrompt(path); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonsrc/configManager.tssrc/storage/repositoryStorage.tssrc/syncManager.tssrc/ui/promptCardsWebview.tssrc/ui/promptDetailsWebview.tssrc/ui/promptTreeProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptTreeProvider.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/syncManager.ts (3)
src/storage/repositoryStorage.ts (1)
getRepositoryStorageDirectory(45-98)src/utils/gitProvider.ts (1)
GitTreeItem(3-9)src/constant.ts (5)
REPO_SYNC_CHAT_MODE_PATH(2-2)REPO_SYNC_CHAT_MODE_LEGACY_PATH(3-3)REPO_SYNC_CHAT_MODE_LEGACY_SINGULAR_PATH(4-4)REPO_SYNC_INSTRUCTIONS_PATH(5-5)REPO_SYNC_PROMPT_PATH(6-6)
src/ui/promptDetailsWebview.ts (1)
src/storage/repositoryStorage.ts (2)
getRepositoryStorageDirectory(45-98)encodeRepositorySlug(13-21)
🔇 Additional comments (1)
src/ui/promptTreeProvider.ts (1)
540-545: Verify underscore-prefixed file filtering is intentional.Line 543 filters out files starting with underscore (
!fileName.startsWith('_')). This prevents files like_template.mdfrom being loaded as prompts. Confirm this is the intended behavior, as some users might use underscore-prefixed files as valid prompts.
|
There are so many changes in this PR, it's hard to know what changes are what, even with multiple commits. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/ui/promptCardsWebview.ts (2)
194-196: Consider removing unused--chatmode-colorCSS variable.The migration to 'agents' is complete, and
--chatmode-color(line 195) appears to be unused. Removing it would reduce maintenance overhead.
885-941: Complex but correct source toggling logic.The sentinel-based state machine correctly handles all edge cases:
- Deselecting the only source
- Reselecting after sentinel
- Transitioning between partial and full selection
Consider adding inline comments documenting the state transitions for future maintainers, as this logic is dense.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/promptCardsWebview.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (8)
src/ui/promptCardsWebview.ts (8)
45-69: Excellent logging and message handling improvements!The structured [WebView]-prefixed logging and the warning for unknown commands improve debuggability and defensive programming. The new
openLocalFolderhandler is properly integrated.
98-135: Robust cross-platform path handling!The path normalization logic (converting backslashes to forward slashes) correctly addresses Windows compatibility. The extensive logging and error recovery with UI refresh ensure consistent state.
165-168: Good reuse of existing command!Delegating to the existing
promptitude.openPromptsFoldercommand avoids code duplication and maintains consistency.
712-724: Sentinel pattern correctly addresses the "no sources selected" state.The
__none__sentinel distinguishes between "all sources enabled" (empty set) and "no sources selected" (sentinel in set), which solves the issue of being unable to deselect the only repository. The logic is correct, though the pattern adds some complexity.
734-741: Chatmode to agents migration completed correctly!All references have been consistently updated to use 'agents' throughout the filtering, grouping, and rendering logic. The past review concerns about dead chatmode code have been properly addressed.
Also applies to: 821-823, 960-966, 1027-1027
1004-1010: Good UX improvement hiding activation button for local prompts.Local prompts are already activated by nature, so hiding the button prevents user confusion. The conditional rendering aligns with the PR objectives.
1107-1111: New openLocalFolder function properly integrated!The function follows the established messaging pattern and correctly integrates with the extension-side handler (lines 165-168).
765-814: Sentinel value correctly reflected in UI rendering.The dropdown rendering logic properly handles all three states (all sources, no sources, partial selection) and maintains consistency between the internal
enabledSourcesset and the displayed checkbox states and labels.
mohamedsedkiyouzbechi
force-pushed
the
msyo/fix-promptitude-ui
branch
from
7ba2a95 to
d3439fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configManager.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (1)
src/configManager.ts (1)
244-244: LGTM: Formatting improvements.The added whitespace improves readability without changing functionality.
Also applies to: 257-257, 280-280
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ui/promptCardsWebview.ts (1)
581-584: Inconsistent color variable usage after agents migration.Line 583 still references
--chatmode-colorfor the active button styling, but the category has been renamed toagents. For consistency, this should use--agents-coloror a more generic variable.🎨 Proposed fix
.action-btn.active-btn { background-color: rgba(var(--vscode-charts-green), 0.15); - color: var(--chatmode-color); - border: 1px solid var(--chatmode-color); + color: var(--agents-color); + border: 1px solid var(--agents-color); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/configManager.tssrc/ui/promptCardsWebview.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configManager.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (8)
src/ui/promptCardsWebview.ts (8)
43-73: LGTM - Enhanced message handling with proper logging and error handling.The additions improve observability and robustness:
- Structured JSON logging for all received messages
- Explicit logging for each command type
- New
openLocalFoldercommand support- Default warning for unknown commands prevents silent failures
98-135: LGTM - Robust path handling for Windows compatibility.The enhanced implementation addresses Windows path issues effectively:
- Path normalization (backslash → forward slash) ensures cross-platform comparison works correctly
- Comprehensive logging aids debugging
- Error handling with UI refresh prevents stale state
- Informative error messages when prompts aren't found
165-169: LGTM - Good reuse of existing command.Delegating to
promptitude.openPromptsFolderavoids code duplication and maintains consistency with the existing folder-opening functionality.
713-724: LGTM - Sentinel value pattern correctly handles "none selected" vs "all sources" distinction.The
__none__sentinel elegantly solves the ambiguity between "no filter applied" (empty set = all sources) and "explicitly no sources selected" (sentinel present = show nothing). The implementation is consistent across:
- Filtering logic (lines 714-716)
- Checkbox rendering (lines 785, 800)
- Label computation (lines 812, 814)
- Toggle behavior (lines 901, 918, 923)
Also applies to: 896-941
734-734: LGTM - Complete chatmode → agents migration.All references to the
chatmodecategory have been consistently updated toagentsthroughout the filtering, rendering, and display logic. The icon choices (🤖 for category, 💬 for type) are appropriate and consistent.Also applies to: 741-741, 821-823, 960-960, 966-966, 1027-1027
1085-1085: LGTM - Appropriate logging for webview context.Using
console.logwith a[Promptitude WebView]prefix is correct here, as the Logger class is not available in the webview's JavaScript context. The prefix helps distinguish webview logs from extension logs during debugging.Also applies to: 1090-1090
1107-1111: LGTM - Consistent message posting pattern.The
openLocalFolderfunction follows the established pattern for webview-to-extension communication, matching the implementation ofopenRepositoryand other similar functions.
995-996: [Rewritten review comment]
[Exactly ONE classification tag]
MatFillion
force-pushed
the
msyo/fix-promptitude-ui
branch
from
d3439fc to
060605e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/ui/promptCardsWebview.ts:
- Around line 995-996: The escaping for Windows paths is wrong: replace the
regex and replacement used to build escapedPath so single backslashes are
escaped; update the code that computes escapedPath from prompt.path (currently
using /\\\\/g and '\\\\\\\\') to use a regex that matches single backslashes
(/\\/g) and a replacement that produces two backslashes in the resulting JS
string (i.e., replace '\' with '\\'), so the value passed into the
onclick/viewPrompt() invocation is correctly preserved on Windows.
- Around line 1004-1010: The template currently wraps the toggle button in a
repositoryUrl check which prevents rendering for local prompts; remove that
conditional so the card-actions block with the action-btn (using prompt.active,
escapedPath and calling togglePrompt(...)) is always rendered for every prompt;
locate the template fragment that references prompt.repositoryUrl and delete the
ternary/conditional wrapper so the div.card-actions and its button are output
unconditionally.
🧹 Nitpick comments (1)
src/ui/promptCardsWebview.ts (1)
194-196: Consider removing legacy--chatmode-colorCSS variable.The
--chatmode-colorvariable (line 195) appears to be legacy code from the chatmode-to-agents migration. Since all references now use--agents-color, the old variable can be safely removed to reduce technical debt.🧹 Proposed cleanup
/* Category colors */ --agents-color: #4CAF50; ---chatmode-color: #4CAF50; --instructions-color: #2196F3; --prompts-color: #FF9800;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/configManager.tssrc/ui/promptCardsWebview.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/configManager.ts (1)
src/utils/logger.ts (1)
error(68-77)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (8)
src/configManager.ts (2)
113-174: Solid implementation with appropriate fallbacks.The getPromptsDirectory() method correctly handles the three-tier resolution strategy (custom path → globalStorageUri-based → platform fallback). The path parsing logic (lines 136-163) includes safety checks and gracefully falls back when parsing fails. Debug logging is properly conditional.
The path parsing fragility concern raised in previous reviews (specifically the 'profiles' string matching on line 136) has been acknowledged by the developer and deferred to a future PR.
242-298: LGTM: Minor whitespace adjustments with no functional changes.The modifications to getUsedProviders() and getRepositoriesByProvider() are purely cosmetic whitespace adjustments that slightly improve readability without altering functionality.
src/ui/promptCardsWebview.ts (6)
18-69: LGTM: Comprehensive logging and message handling.The enhanced logging with JSON stringification and distinct log levels (debug/info/warn) improves debuggability. The addition of
openLocalFoldercommand handling and the default case warning for unknown commands strengthen the message processing logic.
98-135: LGTM: Robust path normalization for Windows compatibility.The path normalization using
.replace(/\\/g, '/')correctly handles Windows backslashes for cross-platform path comparison. The error handling includes a fallback UI refresh to ensure state consistency, and comprehensive logging aids debugging.
165-168: LGTM: Clean delegation to existing command.The
openLocalFoldermethod appropriately reuses the existingpromptitude.openPromptsFoldercommand, avoiding code duplication and maintaining consistency with the broader command infrastructure.
713-724: LGTM: Sentinel pattern correctly handles "no sources selected" state.The
__none__sentinel value cleanly distinguishes between "all sources enabled" (empty set) and "no sources selected" (set containing sentinel). This addresses the PR objective of fixing the "All Sources" dropdown filtering edge case where users couldn't deselect the only repository source.
1025-1032: LGTM: Type icons correctly mapped for agents migration.The
getTypeIconfunction properly maps theagentstype to the speech bubble emoji, completing the migration fromchatmode. The function cleanly handles all valid types with an appropriate default fallback.
1107-1111: LGTM: Clean webview-to-extension messaging.The
openLocalFolderfunction follows the established pattern for webview-to-extension communication, posting a properly structured message that's handled by the message listener in lines 63-66.
| // Escape characters in paths for JavaScript single-quoted strings (Windows compatibility) | ||
| const escapedPath = prompt.path.replace(/\\\\/g, '\\\\\\\\'); |
There was a problem hiding this comment.
Critical: Path escaping regex is incorrect for Windows single backslashes.
The regex pattern /\\\\/g matches only double backslashes (\\), but Windows paths contain single backslashes (e.g., C:\Users\file.txt). This means single backslashes are not escaped, causing them to be interpreted as JavaScript escape sequences in the onclick attribute, which corrupts the path.
Impact: Windows paths will be malformed when passed to viewPrompt(), breaking prompt viewing on Windows—a core issue this PR aims to fix.
🔧 Proposed fix
-// Escape characters in paths for JavaScript single-quoted strings (Windows compatibility)
-const escapedPath = prompt.path.replace(/\\\\/g, '\\\\\\\\');
+// Escape backslashes for JavaScript strings (Windows compatibility)
+const escapedPath = prompt.path.replace(/\\/g, '\\\\');Explanation:
/\\/gmatches each single backslash'\\\\'(four backslashes in source) produces two backslashes in the resulting string, which is what's needed for JavaScript string literals
🤖 Prompt for AI Agents
In @src/ui/promptCardsWebview.ts around lines 995 - 996, The escaping for
Windows paths is wrong: replace the regex and replacement used to build
escapedPath so single backslashes are escaped; update the code that computes
escapedPath from prompt.path (currently using /\\\\/g and '\\\\\\\\') to use a
regex that matches single backslashes (/\\/g) and a replacement that produces
two backslashes in the resulting JS string (i.e., replace '\' with '\\'), so the
value passed into the onclick/viewPrompt() invocation is correctly preserved on
Windows.
| \${prompt.repositoryUrl ? \` | ||
| <div class="card-actions" onclick="event.stopPropagation()"> | ||
| <button class="action-btn \${prompt.active ? 'active-btn' : 'inactive-btn'}" onclick="togglePrompt('\${prompt.path}')" title="\${prompt.active ? 'Click to deactivate' : 'Click to activate'}"> | ||
| <button class="action-btn \${prompt.active ? 'active-btn' : 'inactive-btn'}" onclick="event.stopPropagation(); togglePrompt('\${escapedPath}'); return false;" title="\${prompt.active ? 'Click to deactivate' : 'Click to activate'}"> | ||
| \${prompt.active ? '✓ Activated' : '+ Activate'} | ||
| </button> | ||
| </div> | ||
| \` : ''} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for togglePrompt function and its usage
rg -n "togglePrompt" --type=ts -B 2 -A 2Repository: nventive/Promptitude
Length of output: 2362
🏁 Script executed:
# Search for how local prompts are handled
rg -n "repositoryUrl|local.*prompt" --type=ts -i -B 2 -A 2 | head -100Repository: nventive/Promptitude
Length of output: 6706
🏁 Script executed:
# Get the complete context around lines 1004-1010 in promptCardsWebview.ts
sed -n '980,1030p' src/ui/promptCardsWebview.tsRepository: nventive/Promptitude
Length of output: 2916
🏁 Script executed:
# Search for any comments or documentation about local vs repository prompts
rg -n "local|repository" src/ui/promptCardsWebview.ts -i -B 1 -A 1 | head -60Repository: nventive/Promptitude
Length of output: 2441
🏁 Script executed:
# Check the togglePrompt implementation in the backend
sed -n '98,150p' src/ui/promptCardsWebview.tsRepository: nventive/Promptitude
Length of output: 2605
🏁 Script executed:
# Search for any documentation or comments about local prompt activation behavior
rg -n "local.*activ|activ.*local|always.*active|toggle.*local" --type=ts -iRepository: nventive/Promptitude
Length of output: 534
🏁 Script executed:
# Check if there are test cases or requirements mentioning this behavior
fd -t f -e md -e txt | xargs rg -l "local.*prompt" -i | head -5Repository: nventive/Promptitude
Length of output: 73
🏁 Script executed:
# Look for any comments in the togglePrompt handler or related code
rg -n "private async togglePrompt" --type=ts -A 30Repository: nventive/Promptitude
Length of output: 2775
Remove the repositoryUrl condition to enable toggle buttons for local prompts.
Local prompts have an active state and the backend togglePrompt handler accepts any prompt path without restriction. The conditional rendering at line 1004 prevents users from toggling local prompts via the UI despite full backend support. The button should be shown for all prompts, not just repository-based ones.
Current code (lines 1004-1010)
\${prompt.repositoryUrl ? \`
<div class="card-actions" onclick="event.stopPropagation()">
<button class="action-btn \${prompt.active ? 'active-btn' : 'inactive-btn'}" onclick="event.stopPropagation(); togglePrompt('\${escapedPath}'); return false;" title="\${prompt.active ? 'Click to deactivate' : 'Click to activate'}">
\${prompt.active ? '✓ Activated' : '+ Activate'}
</button>
</div>
\` : ''}
Change to: Remove the prompt.repositoryUrl ? condition so the button renders for all prompts.
🤖 Prompt for AI Agents
In @src/ui/promptCardsWebview.ts around lines 1004 - 1010, The template
currently wraps the toggle button in a repositoryUrl check which prevents
rendering for local prompts; remove that conditional so the card-actions block
with the action-btn (using prompt.active, escapedPath and calling
togglePrompt(...)) is always rendered for every prompt; locate the template
fragment that references prompt.repositoryUrl and delete the ternary/conditional
wrapper so the div.card-actions and its button are output unconditionally.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ui/promptCardsWebview.ts (1)
1083-1090: Consider removing or gating debug console.log statements.These
console.logstatements in the webview script will appear in Developer Tools. If they're for debugging the toggle issue during development, consider removing them before final merge or wrapping in a debug flag.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/promptCardsWebview.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/promptCardsWebview.ts (1)
src/utils/logger.ts (1)
error(68-77)
🔇 Additional comments (9)
src/ui/promptCardsWebview.ts (9)
43-69: LGTM! Well-structured message handling with proper logging.The addition of
[WebView]prefixed logging, the newopenLocalFoldercommand handler, and the default case for unknown commands improve debuggability and robustness.
98-134: LGTM! Solid cross-platform path handling.The path normalization approach correctly handles Windows backslashes for comparison, and the comprehensive logging will aid debugging. The error recovery with UI refresh ensures consistent state.
165-168: LGTM! Clean delegation to existing command.
711-723: LGTM! Sentinel pattern correctly distinguishes "all sources" from "no sources".This effectively fixes the dropdown filtering issue where deselecting the only repository source was not possible before.
764-765: LGTM! Consistent sentinel handling for UI state.The "None Selected" label and count calculations correctly reflect the sentinel state throughout the filter UI.
Also applies to: 811-813
895-936: LGTM! Thorough sentinel management in toggleSource.The logic correctly handles edge cases: single source deselection, last source deselection, and sentinel removal when re-selecting. This is the key fix for the "All Sources" dropdown filtering issue.
1106-1110: LGTM! openLocalFolder function follows established messaging pattern.
194-197: LGTM! CSS variable--agents-colorproperly defined and used.The previously missing CSS variable is now defined in
:rootand consistently applied to agent-related styling.Also applies to: 416-418
1024-1031: LGTM! Clean migration from 'chatmode' to 'agents'.The dead 'chatmode' case has been removed, and the switch correctly handles 'agents' with the appropriate icon.
Summary
Fixes multiple critical bugs affecting the prompts view functionality, particularly on Windows. All fixes have been tested on Windows with screenshots provided.
Bugs Fixed
Testing
Screenshots
PR description images:
Inactive Prompt Path Resolution
"All Sources" Dropdown Selection
Activity Bar Icon Not Displayinge
Changes
promptDetailsWebview.tsfor Windows compatibilitypromptCommands.tsfor direct symlink managementconfigManager.tspromptCardsWebview.tsfor proper dropdown state managementSummary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.