Skip to content

Conversation

goanpeca
Copy link
Collaborator

@goanpeca goanpeca commented Sep 16, 2025

vscode

@goanpeca goanpeca self-assigned this Sep 16, 2025
@goanpeca goanpeca changed the title Implement login and logout functionality VSCode Datalayer: Implement new lexical editor and update login and space tre view Sep 16, 2025
@goanpeca goanpeca changed the title VSCode Datalayer: Implement new lexical editor and update login and space tre view VSCode Datalayer: Implement new lexical editor and update login and space tree view Sep 16, 2025
@goanpeca goanpeca force-pushed the enh/vscode-updates branch 2 times, most recently from da2d1b1 to 81caea7 Compare September 22, 2025 01:55
- Add specific webpack rule for .whl (Python wheel) files
- Ensures wheel files are handled as asset/resource type
- Fixes build errors on Windows CI for pyodide kernel dependencies
- Maintains compatibility with existing pypi resource handling
@goanpeca goanpeca changed the title VSCode Datalayer: Implement new lexical editor and update login and space tree view VSCode Datalayer: Implement new lexical editor and update datalayer notebook Sep 22, 2025
@goanpeca goanpeca marked this pull request as ready for review September 24, 2025 14:34
@Copilot Copilot AI review requested due to automatic review settings September 24, 2025 14:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a new lexical editor and updates the datalayer notebook functionality for the VS Code extension. The changes introduce a comprehensive theme system that integrates with VS Code's theming, a new lexical editor for rich text editing, and enhanced notebook functionality with improved runtime management.

  • Enhanced theme system with VS Code integration and dynamic theme switching
  • New lexical editor with rich text editing capabilities and collaboration support
  • Improved notebook functionality with better runtime selection and Datalayer integration
  • Updated build configuration to support both notebook and lexical editor webviews

Reviewed Changes

Copilot reviewed 68 out of 69 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/vscode/webview/utils.ts Added module documentation and new saveToBytes utility function
packages/vscode/webview/theme/types/index.ts Comprehensive theme type definitions for extensible theme system
packages/vscode/webview/theme/providers/VSCodeThemeProvider.ts VS Code theme provider with color extraction and syntax highlighting
packages/vscode/webview/theme/providers/BaseThemeProvider.ts Base theme provider implementation with common functionality
packages/vscode/webview/theme/mapping/UniversalColorMapper.ts Universal color mapping system between different theme systems
packages/vscode/webview/theme/index.ts Main theme system exports and API
packages/vscode/webview/theme/components/ThemedLoader.tsx VS Code themed loading component
packages/vscode/webview/theme/components/EnhancedJupyterReactTheme.tsx Enhanced theme wrapper with VS Code integration
packages/vscode/webview/theme/components/CodeMirrorThemeInjector.tsx CodeMirror theme injection for syntax highlighting
packages/vscode/webview/theme/codemirror/patchCodeMirror.ts CodeMirror patching for VS Code syntax highlighting
packages/vscode/webview/theme/codemirror/createVSCodeTheme.ts CodeMirror theme creation from VS Code colors
packages/vscode/webview/theme/codemirror/VSCodeCodeMirrorTheme.ts Complete CodeMirror theme matching VS Code
packages/vscode/webview/theme/README.md Comprehensive documentation for the theme system
packages/vscode/webview/serviceManager.ts Enhanced service manager with better documentation
packages/vscode/webview/mockServiceManager.ts Mock service manager for Datalayer notebooks
packages/vscode/webview/messageHandler.ts Enhanced message handling with better logging
packages/vscode/webview/main.ts Simplified entry point with better documentation
packages/vscode/webview/lexicalWebview.tsx Main lexical editor webview implementation
packages/vscode/webview/NotebookVSCode.tsx Completely rewritten notebook component with enhanced functionality
packages/vscode/webview/NotebookToolbar.tsx VS Code styled toolbar for notebook operations
packages/vscode/webview/LexicalToolbar.tsx Comprehensive toolbar for lexical editor formatting
packages/vscode/webview/LexicalEditor.tsx Main lexical editor with VS Code theme integration
packages/vscode/webview/LexicalEditor.css Complete CSS styling for lexical editor with VS Code theme variables
packages/vscode/webpack.config.js Enhanced webpack configuration with CodeMirror deduplication
packages/vscode/tsconfig.json Updated TypeScript configuration
packages/vscode/tsconfig.doc.json New TypeScript configuration for documentation
packages/vscode/src/util.ts Enhanced utility functions with documentation
packages/vscode/src/spaces/spacesTreeProvider.ts Tree provider for Datalayer spaces view
packages/vscode/src/spaces/spacerApiService.ts Comprehensive API service for Datalayer platform
packages/vscode/src/spaces/spaceItem.ts Data models for spaces and documents
packages/vscode/src/spaces/documentBridge.ts Document lifecycle management bridge

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +33 to +36
console.log(
'[VSCodeThemeProvider] Initializing with color mode:',
colorMode,
);
Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more structured logging approach or reducing the verbosity of console.log statements in production code. Multiple console.log statements throughout the file may impact performance and clutter logs.

Suggested change
console.log(
'[VSCodeThemeProvider] Initializing with color mode:',
colorMode,
);
// Removed verbose console.log to avoid cluttering production logs.

Copilot uses AI. Check for mistakes.

Comment on lines +225 to +230
console.log('[VSCodeThemeProvider] Detected VS Code colors:', {
count: this._vscodeColors.size,
sample: Array.from(this._vscodeColors.entries()).slice(0, 5),
editorBg: this._vscodeColors.get('--vscode-editor-background'),
editorFg: this._vscodeColors.get('--vscode-editor-foreground'),
});
Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The console.log statement creates a potentially large object for debugging. Consider using a debug flag or environment variable to control verbose logging to avoid performance overhead in production.

Suggested change
console.log('[VSCodeThemeProvider] Detected VS Code colors:', {
count: this._vscodeColors.size,
sample: Array.from(this._vscodeColors.entries()).slice(0, 5),
editorBg: this._vscodeColors.get('--vscode-editor-background'),
editorFg: this._vscodeColors.get('--vscode-editor-foreground'),
});
if (
(typeof window !== 'undefined' && (window as any).DEBUG) ||
(typeof process !== 'undefined' && process.env && process.env.DEBUG)
) {
console.log('[VSCodeThemeProvider] Detected VS Code colors:', {
count: this._vscodeColors.size,
sample: Array.from(this._vscodeColors.entries()).slice(0, 5),
editorBg: this._vscodeColors.get('--vscode-editor-background'),
editorFg: this._vscodeColors.get('--vscode-editor-foreground'),
});
}

Copilot uses AI. Check for mistakes.

Comment on lines +292 to +295
const isDark = getColor('--vscode-editor-background', '#ffffff')
.toLowerCase()
.includes('2');

Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dark theme detection logic is fragile and unreliable. Checking if a color string includes '2' is not a proper way to determine if a theme is dark. This could misclassify colors like '#ffffff' or '#f2f2f2'.

Suggested change
const isDark = getColor('--vscode-editor-background', '#ffffff')
.toLowerCase()
.includes('2');
// Helper to determine if a color is "dark" based on luminance
function isColorDark(hexColor: string): boolean {
// Remove leading '#' if present
hexColor = hexColor.trim();
if (hexColor.startsWith('#')) {
hexColor = hexColor.slice(1);
}
// Support short hex (#fff) and long hex (#ffffff)
if (hexColor.length === 3) {
hexColor = hexColor[0] + hexColor[0] +
hexColor[1] + hexColor[1] +
hexColor[2] + hexColor[2];
}
if (hexColor.length !== 6) {
// Fallback: treat as light
return false;
}
const r = parseInt(hexColor.slice(0, 2), 16);
const g = parseInt(hexColor.slice(2, 4), 16);
const b = parseInt(hexColor.slice(4, 6), 16);
// Relative luminance formula
const luminance = (0.2126 * r + 0.7152 * g + 0.0722 * b) / 255;
// Threshold: < 0.5 is dark
return luminance < 0.5;
}
const editorBg = getColor('--vscode-editor-background', '#ffffff');
const isDark = isColorDark(editorBg);

Copilot uses AI. Check for mistakes.

Comment on lines +939 to +980
window.debugNotebook = function () {
const all = document.querySelectorAll('*');
const results: any[] = [];

all.forEach((el: Element) => {
const style = getComputedStyle(el);
const bg = style.backgroundColor;

// Check for black or near-black backgrounds
if (bg && bg !== 'rgba(0, 0, 0, 0)' && bg !== 'transparent') {
const match = bg.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
if (match) {
const [_, r, g, b] = match;
if (parseInt(r) < 50 && parseInt(g) < 50 && parseInt(b) < 50) {
results.push({
element: el,
tag: el.tagName,
id: el.id,
classes: el.className,
backgroundColor: bg,
parent: el.parentElement?.tagName,
parentId: el.parentElement?.id,
parentClasses: el.parentElement?.className,
});
}
}
}
});

console.log('Dark elements found:', results);
results.forEach(r => {
console.log(
`${r.tag}#${r.id || 'no-id'}.${r.classes || 'no-class'} = ${r.backgroundColor}`,
);
});
return results;
};

// Log that the debug function is available
console.log(
'[NotebookVSCode] Debug function installed on window.debugNotebook',
);
Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global debug functions should not be added to the window object in production code. Consider removing this debug functionality or wrapping it in a development-only check.

Suggested change
window.debugNotebook = function () {
const all = document.querySelectorAll('*');
const results: any[] = [];
all.forEach((el: Element) => {
const style = getComputedStyle(el);
const bg = style.backgroundColor;
// Check for black or near-black backgrounds
if (bg && bg !== 'rgba(0, 0, 0, 0)' && bg !== 'transparent') {
const match = bg.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
if (match) {
const [_, r, g, b] = match;
if (parseInt(r) < 50 && parseInt(g) < 50 && parseInt(b) < 50) {
results.push({
element: el,
tag: el.tagName,
id: el.id,
classes: el.className,
backgroundColor: bg,
parent: el.parentElement?.tagName,
parentId: el.parentElement?.id,
parentClasses: el.parentElement?.className,
});
}
}
}
});
console.log('Dark elements found:', results);
results.forEach(r => {
console.log(
`${r.tag}#${r.id || 'no-id'}.${r.classes || 'no-class'} = ${r.backgroundColor}`,
);
});
return results;
};
// Log that the debug function is available
console.log(
'[NotebookVSCode] Debug function installed on window.debugNotebook',
);
if (process.env.NODE_ENV !== 'production') {
window.debugNotebook = function () {
const all = document.querySelectorAll('*');
const results: any[] = [];
all.forEach((el: Element) => {
const style = getComputedStyle(el);
const bg = style.backgroundColor;
// Check for black or near-black backgrounds
if (bg && bg !== 'rgba(0, 0, 0, 0)' && bg !== 'transparent') {
const match = bg.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
if (match) {
const [_, r, g, b] = match;
if (parseInt(r) < 50 && parseInt(g) < 50 && parseInt(b) < 50) {
results.push({
element: el,
tag: el.tagName,
id: el.id,
classes: el.className,
backgroundColor: bg,
parent: el.parentElement?.tagName,
parentId: el.parentElement?.id,
parentClasses: el.parentElement?.className,
});
}
}
}
});
console.log('Dark elements found:', results);
results.forEach(r => {
console.log(
`${r.tag}#${r.id || 'no-id'}.${r.classes || 'no-class'} = ${r.backgroundColor}`,
);
});
return results;
};
// Log that the debug function is available
console.log(
'[NotebookVSCode] Debug function installed on window.debugNotebook',
);
}

Copilot uses AI. Check for mistakes.

Comment on lines +284 to +290
if ($isHeadingNode(element) && element.getTag() === headingTag) {
// If already this heading type, convert to paragraph
const paragraph = element.replace(
$createHeadingNode('h1').replace($createHeadingNode('h1')),
true,
);
} else {
Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paragraph conversion logic is incorrect. The code creates a new h1 node and immediately replaces it with another h1 node, then uses that result to replace the current element. This doesn't actually convert to a paragraph.

Copilot uses AI. Check for mistakes.

Comment on lines +162 to +169
// Rule for pyodide kernel wheel files
{
test: /\.whl$/,
type: 'asset/resource',
generator: {
filename: 'pypi/[name][ext]',
},
},
Copy link
Preview

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate pyodide/pypi rules (lines 162-169 and 171-177) handle different file patterns but both use similar configurations. Consider consolidating these rules or adding comments to clarify the distinction.

Copilot uses AI. Check for mistakes.

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thx @goanpeca

@echarles echarles merged commit a6e961c into datalayer:main Sep 24, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants