issues/2: Add Vscode Insiders Support & Improve Logging - #3
Conversation
Added the path to the vscode insiders workbench.html to possible paths in the `findWorkbenchPath()` function.
Introduces a dedicated output channel for logging extension messages and errors. Adds debug report generation to assist troubleshooting when the workbench.html path is not detected. Updates made to README `Troubleshooting / FAQ` and `Bug Reports & Feature Requests` sections.
WalkthroughThe changes introduce a centralized logging mechanism using a VS Code Output Channel, replacing all console logging with structured LOGGER methods. A new utility function generates detailed debug reports for troubleshooting. The README is updated to instruct users to include output channel logs in bug reports. No core logic or control flow is altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Extension
participant VSCode Output Channel
User->>Extension: Activates extension
Extension->>VSCode Output Channel: LOGGER.info("Extension activated")
Extension->>Extension: Attempts to find workbench.html
alt Path not found
Extension->>Extension: generateDebugReport()
Extension->>VSCode Output Channel: LOGGER.error(debug report)
end
Extension->>VSCode Output Channel: Logs all info/warn/error/debug messages
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (5)
out/extension.js (3)
154-158: Incomplete migration from console logging to LOGGER.Several
console.log,console.error, andconsole.warncalls remain in the temporary file cleanup sections. For consistency with the PR objectives, these should also use the LOGGER.Replace the remaining console calls:
- console.log(`Successfully deleted temp file: ${tempFileUri.fsPath}`); + LOGGER.info(`Successfully deleted temp file: ${tempFileUri.fsPath}`); } catch (deleteError) { - console.error(`Failed to delete temp file: ${deleteError.message}`); + LOGGER.error(`Failed to delete temp file: ${deleteError.message}`); }- console.log('Sudo command successful.'); + LOGGER.info('Sudo command successful.'); // Attempt to clean up the temporary file after successful move try { await vscode.workspace.fs.delete(tempFileUri); - console.log(`Successfully deleted temp file after move: ${tempFileUri.fsPath}`); + LOGGER.info(`Successfully deleted temp file after move: ${tempFileUri.fsPath}`); } catch (deleteError) { // Log warning if deletion fails (might already be gone) - console.warn(`Could not delete temp file after successful move (might already be gone): ${deleteError.message}`); + LOGGER.warn(`Could not delete temp file after successful move (might already be gone): ${deleteError.message}`);Also applies to: 166-172
489-508: Console calls remain in the activate and deactivate functions.Multiple console calls in the activation logic should be replaced with LOGGER for consistency:
- Lines 490, 499, 503, 507: Theme application logs
- Lines 527, 533, 545, 553, 561: Version and configuration logs
- Line 571: Activation success log
- Line 579: Deactivation log
Also applies to: 527-527, 533-533, 545-545, 553-553, 561-561, 571-571, 579-579
278-285: Replace all remaining console calls with LOGGER methodsTo maintain consistency with the PR’s objective of standardizing logging, every
console.log,console.warn, andconsole.errorinout/extension.jsshould be migrated to the correspondingLOGGERmethod:• Info-level (
console.log→LOGGER.info): lines 154, 163, 167, 278, 285, 351, 356, 490, 495, 500, 533, 541, 555, 559, 565, 571, 579
• Warn-level (console.warn→LOGGER.warn): lines 171, 336
• Error-level (console.error→LOGGER.error): lines 157, 298, 308, 367, 381, 527
• Inline error handlers in catches (e.g.applyStyles().catch(err => console.error(...))): lines 543, 561Please update each occurrence to use the appropriate
LOGGERcall.src/extension.ts (2)
124-127: Replace remaining console calls with LOGGER in temp file operations.For consistency with the new logging approach, replace these console calls:
- console.log(`Successfully deleted temp file: ${tempFileUri.fsPath}`); + LOGGER.info(`Successfully deleted temp file: ${tempFileUri.fsPath}`); } catch (deleteError: any) { - console.error(`Failed to delete temp file: ${deleteError.message}`); + LOGGER.error(`Failed to delete temp file: ${deleteError.message}`);- console.log('Sudo command successful.'); + LOGGER.info('Sudo command successful.'); // Attempt to clean up the temporary file after successful move try { await vscode.workspace.fs.delete(tempFileUri); - console.log(`Successfully deleted temp file after move: ${tempFileUri.fsPath}`); + LOGGER.info(`Successfully deleted temp file after move: ${tempFileUri.fsPath}`); } catch (deleteError: any) { // Log warning if deletion fails (might already be gone) - console.warn(`Could not delete temp file after successful move (might already be gone): ${deleteError.message}`); + LOGGER.warn(`Could not delete temp file after successful move (might already be gone): ${deleteError.message}`);Also applies to: 133-141
262-268: Multiple console calls remain; migrate them to LOGGERTo complete the PR goal of using a dedicated output channel, all
console.log,console.error, andconsole.warncalls insrc/extension.tsmust be replaced with appropriateLOGGERmethods. Please update the following occurrences:
File operations (temp files & sudo)
- Line 124:
console.log('Successfully deleted temp file: …')- Line 126:
console.error('Failed to delete temp file: …')- Line 133:
console.log('Sudo command successful.')- Line 137:
console.log('Successfully deleted temp file after move: …')- Line 140:
console.warn('Could not delete temp file after successful move: …')Theme management
- Line 262:
console.log('Storing current theme: …')- Line 268:
console.log('Auto theme apply skipped. …')- Line 344:
console.log('Storing previous theme: …')- Line 348:
console.log('Theme reversion skipped. …')- Line 499:
console.log('Applying pending theme change to: …')- Line 503:
console.log('Applying pending theme revert to: …')- Line 507:
console.log('No pending theme changes to apply on activation')Style injection & removal errors
- Line 282:
console.error('Failed to write workbench.html.')- Line 294:
console.error('Error applying styles: …')- Line 326:
console.warn('Injection markers not found…')- Line 360:
console.error('Failed to write workbench.html during style removal.')- Line 376:
console.error('Error removing styles: …')Version/update logs
- Line 545:
console.error('Failed to read package.json: …')- Line 553:
console.log('Version change detected (Stored: …)')- Line 561:
console.log('Re-applying styles after update.')- Line 563:
applyStyles().catch(err => console.error('Error during automatic style re-application after update:', err) );Config‐change notifications
- Line 578:
console.log('Relevant configuration changed.')- Line 582:
console.log('Styles are applied, re-applying due to config change.')- Line 584:
applyStyles().catch(err => console.error('Error during automatic style re-application after config change:', err) );- Line 587:
console.log('Config changed, but styles are not currently applied. No action needed.')Activation/Deactivation
- Line 595:
console.log('… activated successfully (Version: …)')- Line 604:
console.log('… deactivated.')Replace each with the corresponding
LOGGER.info(),LOGGER.error(), orLOGGER.warn()call so all extension messages use the shared output channel.
🧹 Nitpick comments (1)
src/extension.ts (1)
382-484: Excellent implementation of the debug report utility.The
generateDebugReportfunction provides comprehensive system and installation information that will be invaluable for troubleshooting. The error handling ensures the function doesn't fail even if some information can't be retrieved.Consider adding VS Code extension host information to help identify whether the extension is running in a regular window vs. a remote/WSL context:
Extension Host: ${vscode.env.isNewAppInstall ? 'New Install' : 'Existing'} UI Kind: ${vscode.env.uiKind === vscode.UIKind.Desktop ? 'Desktop' : 'Web'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
out/extension.js.mapis excluded by!**/*.map
📒 Files selected for processing (3)
README.md(3 hunks)out/extension.js(8 hunks)src/extension.ts(9 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
out/extension.js
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 405-405: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 406-406: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
src/extension.ts
[error] 404-404: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 405-405: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (3)
README.md (1)
104-106: Documentation updates align well with the new logging functionality.The new FAQ entry clearly explains how to access the extension logs, and the bug report instructions appropriately request users to include these logs. This will significantly improve the debugging and support process.
Also applies to: 125-125
out/extension.js (1)
72-107: Excellent implementation of VS Code Insiders support and comprehensive error reporting.The addition of the
electron-browserpath for VS Code Insiders is correctly implemented. The enhanced logging provides valuable debugging information, and the debug report generation on failure will greatly assist in troubleshooting path-related issues.src/extension.ts (1)
8-11: Well-implemented logging infrastructure and VS Code Insiders support.The LOGGER initialization with the output channel is properly configured, and the enhanced path discovery with detailed logging will greatly improve debugging capabilities. The VS Code Insiders path addition is correctly placed.
Also applies to: 37-71
| readAccess = !!(mode & 0o444) ? "True" : "False"; | ||
| writeAccess = !!(mode & 0o222) ? "True" : "False"; |
There was a problem hiding this comment.
Remove redundant double negation as flagged by static analysis.
The double negation (!!) is unnecessary here since the result is being converted to a string anyway.
Apply this fix:
- readAccess = !!(mode & 0o444) ? "True" : "False";
- writeAccess = !!(mode & 0o222) ? "True" : "False";
+ readAccess = (mode & 0o444) ? "True" : "False";
+ writeAccess = (mode & 0o222) ? "True" : "False";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readAccess = !!(mode & 0o444) ? "True" : "False"; | |
| writeAccess = !!(mode & 0o222) ? "True" : "False"; | |
| readAccess = (mode & 0o444) ? "True" : "False"; | |
| writeAccess = (mode & 0o222) ? "True" : "False"; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 405-405: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 406-406: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🤖 Prompt for AI Agents
In out/extension.js at lines 405 to 406, remove the redundant double negation
(!!) from the assignments to readAccess and writeAccess. Since the ternary
operator converts the result to the strings "True" or "False", the double
negation is unnecessary. Simply check the bitwise condition directly in the
ternary expression without using !!.
| readAccess = !!(mode & 0o444) ? "True" : "False"; | ||
| writeAccess = !!(mode & 0o222) ? "True" : "False"; |
There was a problem hiding this comment.
Remove redundant double negation operators.
The static analysis tool correctly identified unnecessary double negation.
- readAccess = !!(mode & 0o444) ? "True" : "False";
- writeAccess = !!(mode & 0o222) ? "True" : "False";
+ readAccess = (mode & 0o444) ? "True" : "False";
+ writeAccess = (mode & 0o222) ? "True" : "False";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readAccess = !!(mode & 0o444) ? "True" : "False"; | |
| writeAccess = !!(mode & 0o222) ? "True" : "False"; | |
| readAccess = (mode & 0o444) ? "True" : "False"; | |
| writeAccess = (mode & 0o222) ? "True" : "False"; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 404-404: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 405-405: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🤖 Prompt for AI Agents
In src/extension.ts around lines 404 to 405, remove the redundant double
negation operators (!!) used before the bitwise checks on mode. Instead of using
!!(mode & 0o444) and !!(mode & 0o222), directly use the truthiness of (mode &
0o444) and (mode & 0o222) to assign "True" or "False" to readAccess and
writeAccess respectively.
0x3at
changed the title
|
Bump |
1 participant
feat: Add VS Code Insiders Support & Enhanced Logging
This PR introduces support for VS Code Insiders by extending path resolution to include
out/vs/code/electron-browser/workbench/workbench.html. This path reflects the structure used by Insiders, which differs from stable releases by using theelectron-browserdirectory instead ofelectron-sandbox.Key Improvements
VS Code Insiders Compatibility:
The extension now recognizes and adapts to the unique directory structure of VS Code Insiders, ensuring proper functionality across both stable and insiders builds.
Dedicated Logging Output Channel:
A specialized output channel has been added for extension logs, separating them from general output and making debugging more straightforward.
Debug Report Generation:
Introduces a utility to generate detailed debug reports, specifically triggered when the
workbench.htmlpath cannot be located. This will help users and maintainers diagnose issues and add support for forks or custom builds more easily.Benefits
Summary by CodeRabbit
New Features
Documentation