Add LSP for lexical, fix progress bar, improve tools! #478
Conversation
goanpeca
force-pushed
the
enh/tools-enhance
branch
2 times, most recently
from
9ca0b57 to
0c2c1ef
Compare
goanpeca
force-pushed
the
enh/tools-enhance
branch
2 times, most recently
from
d3fc1ad to
4d6da74
Compare
goanpeca
changed the title
goanpeca
force-pushed
the
enh/tools-enhance
branch
3 times, most recently
from
45ae20d to
2a1761d
Compare
goanpeca
force-pushed
the
enh/tools-enhance
branch
from
2a1761d to
9825d2b
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds LSP (Language Server Protocol) support for Lexical code cells, fixes progress bar display and spacing in output cells, and significantly improves the tool system for both notebook and Lexical operations.
Changes:
- Adds comprehensive LSP integration for Lexical with Python and Markdown support (tab completions, document sync, completion menu)
- Refactors progress bar to be always visible with consistent spacing, preventing UI jumps
- Renames and improves deletion operations (deleteCell → deleteCells, deleteBlock → deleteBlocks) with better validation
- Enhances Zod schemas with preprocessing for LLM-friendly coercion (string to number conversion)
- Adds support for new Lexical block types (YouTube, Excalidraw, Table, Collapsible) with improved markdown parsing
- Improves completion provider injection in NotebookBase for extensibility
Reviewed changes
Copilot reviewed 63 out of 65 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/lexical/src/plugins/lspTypes.ts | Defines LSP types, completion items, and message interfaces for Lexical integration |
| packages/lexical/src/plugins/LSPTabCompletionProvider.ts | Implements LSP provider for fetching completions from extension host via postMessage |
| packages/lexical/src/plugins/LSPTabCompletionPlugin.tsx | Provides dropdown Tab completion menu integrated with LSP servers |
| packages/lexical/src/plugins/LSPDocumentSyncPlugin.tsx | Synchronizes JupyterInputNode content with extension host for LSP analysis |
| packages/lexical/src/plugins/LSPCompletionMenu.tsx | Custom dropdown menu component for LSP completions with keyboard navigation |
| packages/react/src/components/output/Output.tsx | Refactors progress bar to floating absolute position with consistent 3px height |
| packages/react/src/components/kernel/KernelActionMenu.tsx | Makes menu always visible with disabled states instead of conditional rendering |
| packages/react/src/components/cell/Cell.tsx | Reduces output area padding from 30px to 2px for tighter spacing |
| packages/react/src/tools/schemas/*.ts | Renames deleteCell → deleteCells with array coercion preprocessing |
| packages/lexical/src/tools/schemas/*.ts | Renames deleteBlock → deleteBlocks, adds metadata preprocessing for JSON strings |
| packages/react/src/tools/core/zodUtils.ts | Improves schema unwrapping to handle ZodEffects, ZodPipe, and ZodRecord types |
| packages/lexical/src/state/LexicalAdapter.ts | Adds insertBlock support for collapsibles, YouTube, tables with markdown parsing |
| packages/lexical/src/tools/operations/listAvailableBlocks.ts | Removes category field, adds detailed block schemas for new types |
| packages/react/src/components/notebook/NotebookBase.tsx | Adds providers prop for injecting custom Tab completion providers |
| patches/@jupyterlab+apputils-extension+4.5.0.patch | Comments out react-toastify import to fix Next.js webpack resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let iterations = 0; | ||
| while (innerType && iterations < 10) { | ||
| iterations++; | ||
| const effectType = innerType._def?.type || innerType.def?.type; | ||
| const effectTypeName = innerType._def?.typeName; | ||
|
|
||
| if ( | ||
| effectType === 'pipe' || | ||
| effectType === 'effects' || | ||
| effectTypeName === 'ZodEffects' | ||
| ) { | ||
| // Zod v4 pipe: output schema is in _def.out | ||
| // Zod v3 effects: output schema is in _def.schema | ||
| innerType = | ||
| innerType._def?.out || | ||
| innerType._def?.schema || | ||
| innerType.def?.schema || | ||
| innerType; | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The while loop for unwrapping ZodEffects/pipe has a hardcoded iteration limit of 10. While this prevents infinite loops, it silently fails if more than 10 layers of nesting exist. Consider either: 1) Adding a warning/error when the limit is reached, 2) Increasing the limit with documentation of the expected maximum depth, or 3) Using a Set to track visited schemas and detect cycles explicitly instead of relying on an arbitrary iteration count.
| `2) Then insert nested blocks with properties.collapsible set to that blockId. ` + | ||
| `Use afterId for positioning (TOP/BOTTOM/blockId), not properties.collapsible.`, |
There was a problem hiding this comment.
The error message references "properties.collapsible" but the code actually uses "metadata.collapsible" (as seen on line 222). This inconsistency between the code and error message will confuse users when they encounter this error. Update the error message to use "metadata.collapsible" instead of "properties.collapsible" to match the actual implementation.
| `2) Then insert nested blocks with properties.collapsible set to that blockId. ` + | |
| `Use afterId for positioning (TOP/BOTTOM/blockId), not properties.collapsible.`, | |
| `2) Then insert nested blocks with metadata.collapsible set to that blockId. ` + | |
| `Use afterId for positioning (TOP/BOTTOM/blockId), not metadata.collapsible.`, |
| console.log('[OperationRunner] 🚀 execute CALLED'); | ||
| console.log('[OperationRunner] Operation:', operation.name); | ||
| console.log('[OperationRunner] Params:', params); | ||
| console.log('[OperationRunner] Context:', context); | ||
|
|
||
| // Execute operation (returns pure typed data) | ||
| console.log('[OperationRunner] 📞 Calling operation.execute...'); | ||
| const result = await operation.execute(params, context); | ||
| console.log('[OperationRunner] ✅ Operation completed, result:', result); | ||
|
|
||
| // Apply formatting based on context.format | ||
| return formatResponse(result, context.format); | ||
| console.log('[OperationRunner] 🎨 Applying formatting...'); | ||
| const formatted = formatResponse(result, context.format); | ||
| console.log('[OperationRunner] ✅ Formatted result:', formatted); | ||
|
|
||
| return formatted; |
There was a problem hiding this comment.
Console.log statements should be removed before merging to production. These debug logs will clutter the console output for end users and may expose internal implementation details. Consider using a proper logging framework with configurable log levels, or remove these logs entirely if they were only needed during development.
| setTimeout(() => { | ||
| this._editor.update(() => { | ||
| const root = $getRoot(); | ||
| const children = root.getChildren(); | ||
|
|
||
| // Find the node we just inserted | ||
| const insertedNode = children.find( | ||
| c => c.getKey() === result.blockId, | ||
| ); | ||
|
|
||
| if (!insertedNode) { | ||
| resolve({ | ||
| success: false, | ||
| error: `Could not find inserted node ${result.blockId}`, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Remove from root | ||
| insertedNode.remove(); | ||
|
|
||
| // Append to collapsible content | ||
| contentNode.append(insertedNode); | ||
|
|
||
| resolve({ | ||
| success: true, | ||
| blockId: result.blockId, | ||
| message: `Block of type '${block.block_type}' inserted inside collapsible ${collapsibleId}`, | ||
| }); | ||
| }); | ||
| }, 50); // Wait a bit longer to ensure command-based insertions complete |
There was a problem hiding this comment.
The nested async operation with setTimeout(50ms) creates a potential race condition. If the user performs another action or the editor state changes during this 50ms delay, the insertBlock call may fail or insert at an unexpected location. Consider using editor.update() with proper error handling instead of setTimeout, or document why this delay is necessary and under what conditions it might fail.
| setTimeout(() => { | |
| this._editor.update(() => { | |
| const root = $getRoot(); | |
| const children = root.getChildren(); | |
| // Find the node we just inserted | |
| const insertedNode = children.find( | |
| c => c.getKey() === result.blockId, | |
| ); | |
| if (!insertedNode) { | |
| resolve({ | |
| success: false, | |
| error: `Could not find inserted node ${result.blockId}`, | |
| }); | |
| return; | |
| } | |
| // Remove from root | |
| insertedNode.remove(); | |
| // Append to collapsible content | |
| contentNode.append(insertedNode); | |
| resolve({ | |
| success: true, | |
| blockId: result.blockId, | |
| message: `Block of type '${block.block_type}' inserted inside collapsible ${collapsibleId}`, | |
| }); | |
| }); | |
| }, 50); // Wait a bit longer to ensure command-based insertions complete | |
| this._editor.update(() => { | |
| const root = $getRoot(); | |
| const children = root.getChildren(); | |
| // Find the node we just inserted | |
| const insertedNode = children.find( | |
| c => c.getKey() === result.blockId, | |
| ); | |
| if (!insertedNode) { | |
| resolve({ | |
| success: false, | |
| error: `Could not find inserted node ${result.blockId}`, | |
| }); | |
| return; | |
| } | |
| // Remove from root | |
| insertedNode.remove(); | |
| // Append to collapsible content | |
| contentNode.append(insertedNode); | |
| resolve({ | |
| success: true, | |
| blockId: result.blockId, | |
| message: `Block of type '${block.block_type}' inserted inside collapsible ${collapsibleId}`, | |
| }); | |
| }); |
| <Box | ||
| sx={{ | ||
| position: 'absolute', | ||
| top: '-5px', | ||
| left: 0, | ||
| right: 0, | ||
| zIndex: 10, | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'space-between', | ||
| margin: 0, | ||
| padding: 0, | ||
| '& .jp-OutputArea': { | ||
| fontSize: '10px', | ||
| backgroundColor: 'transparent', | ||
| transition: 'opacity 0.2s ease-in-out', | ||
| opacity: 1, | ||
| pointerEvents: 'auto', | ||
| height: '3px', | ||
| '& span[data-component="ProgressBar"]': { | ||
| height: '3px !important', | ||
| backgroundColor: 'transparent !important', | ||
| }, | ||
| '& .jp-OutputPrompt': { | ||
| // display: 'none', | ||
| '& .Progress': { | ||
| height: '3px !important', | ||
| backgroundColor: 'transparent !important', | ||
| }, | ||
| '& .jp-OutputArea-prompt': { | ||
| display: 'none', | ||
| // width: '0px', | ||
| '& .Progress-item': { | ||
| height: '3px !important', | ||
| }, | ||
| '& pre': { | ||
| fontSize: '12px', | ||
| wordBreak: 'break-all', | ||
| wordWrap: 'break-word', | ||
| whiteSpace: 'pre-wrap', | ||
| '& [role="progressbar"]': { | ||
| height: '3px !important', | ||
| }, | ||
| }} | ||
| > | ||
| {(() => { | ||
| const currentAdapter = adapter || propsAdapter; | ||
| return lumino ? ( | ||
| currentAdapter ? ( | ||
| <Lumino>{currentAdapter.outputArea}</Lumino> | ||
| ) : null | ||
| ) : ( | ||
| outputs && ( | ||
| <> | ||
| {outputs.map((output: IOutput, index: number) => { | ||
| return <OutputRenderer key={index} output={output} />; | ||
| })} | ||
| </> | ||
| ) | ||
| ); | ||
| })()} | ||
| <Box | ||
| flexGrow={1} | ||
| sx={{ | ||
| height: '3px', | ||
| backgroundColor: | ||
| kernel && kernelStatus !== 'idle' | ||
| ? 'rgba(128, 128, 128, 0.2)' | ||
| : 'transparent', | ||
| position: 'relative', | ||
| }} | ||
| > | ||
| {kernel && kernelStatus !== 'idle' && showKernelProgressBar && ( | ||
| <Box | ||
| sx={{ | ||
| position: 'absolute', | ||
| top: 0, | ||
| left: 0, | ||
| right: 0, | ||
| height: '3px', | ||
| backgroundColor: 'transparent', | ||
| '& > *': { | ||
| backgroundColor: 'transparent !important', | ||
| }, | ||
| }} | ||
| > | ||
| <KernelProgressBar /> | ||
| </Box> | ||
| )} | ||
| </Box> | ||
| <Box | ||
| sx={{ | ||
| marginLeft: '4px', | ||
| height: '3px', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| '& button': { | ||
| height: '12px !important', | ||
| width: '16px !important', | ||
| minWidth: '16px !important', | ||
| padding: '0 !important', | ||
| borderRadius: '2px !important', | ||
| }, | ||
| '& button svg': { | ||
| width: '10px !important', | ||
| height: '10px !important', | ||
| }, | ||
| }} | ||
| > | ||
| <KernelActionMenu | ||
| kernel={kernel} | ||
| outputAdapter={adapter} | ||
| onClearOutputs={handleClearOutputs} | ||
| /> | ||
| </Box> | ||
| </Box> |
There was a problem hiding this comment.
The floating controls are positioned absolutely with a fixed height of 3px and top offset of -5px. This creates a very narrow interactive area that may be difficult to click, especially for users with accessibility needs or on touch devices. Consider increasing the clickable area or adding padding to make the controls easier to interact with. The 3px height seems too small for usable UI controls.
| // FIX: Move cursor to end of inserted text | ||
| // Select the entire inserted text node | ||
| textNode.select(); | ||
| // Then collapse the selection to the end | ||
| const newSelection = $getSelection(); | ||
| if ($isRangeSelection(newSelection)) { | ||
| newSelection.anchor.set( | ||
| textNode.getKey(), | ||
| currentCompletion.length, | ||
| 'text', | ||
| ); | ||
| newSelection.focus.set( | ||
| textNode.getKey(), | ||
| currentCompletion.length, | ||
| 'text', | ||
| ); | ||
| } |
There was a problem hiding this comment.
The cursor positioning fix uses hardcoded values for anchor and focus positions. The fix sets both anchor and focus to currentCompletion.length, which assumes the completion is inserted at the current position. However, if the completion replaces existing text or is inserted at a different position, this could place the cursor incorrectly. Consider calculating the correct cursor position based on the actual insertion point and length of replaced text, if any.
| console.log('[LexicalState] 🔍 listAvailableBlocks CALLED with:', { id }); | ||
|
|
||
| // Delegate to adapter (following consistent pattern with all other operations) | ||
| const params = typeof id === 'object' ? id : { id }; | ||
| console.log('[LexicalState] 📦 Processed params:', params); | ||
|
|
||
| // Special case: this operation is static and doesn't require a document | ||
| // If no document is found, call the operation directly | ||
| const adapter = get().lexicals.get(params.id as string)?.adapter; | ||
| console.log('[LexicalState] 🔧 Adapter found?', !!adapter); | ||
|
|
||
| if (!adapter) { | ||
| console.log('[LexicalState] 🚀 Calling operation directly (no adapter)'); | ||
| // Call operation directly without adapter (static operation) | ||
| const { listAvailableBlocksOperation } = | ||
| await import('../tools/operations/listAvailableBlocks'); | ||
| console.log('[LexicalState] 📥 Operation imported, executing...'); | ||
| const result = await listAvailableBlocksOperation.execute( | ||
| { type: 'all' }, | ||
| { documentId: 'static', executor: null as any }, | ||
| ); | ||
| console.log('[LexicalState] ✅ Operation result:', result); | ||
| return result; | ||
| } | ||
|
|
||
| console.log('[LexicalState] 🔗 Delegating to adapter'); | ||
| const result = await adapter.listAvailableBlocks(); | ||
| console.log('[LexicalState] ✅ Adapter result:', result); |
There was a problem hiding this comment.
Console.log statements should be removed before merging to production. These debug logs are excessive and will clutter the console. Consider using a proper logging framework with configurable log levels, or remove these logs entirely if they were only needed during development.
| console.log('[LexicalState] 🔍 listAvailableBlocks CALLED with:', { id }); | |
| // Delegate to adapter (following consistent pattern with all other operations) | |
| const params = typeof id === 'object' ? id : { id }; | |
| console.log('[LexicalState] 📦 Processed params:', params); | |
| // Special case: this operation is static and doesn't require a document | |
| // If no document is found, call the operation directly | |
| const adapter = get().lexicals.get(params.id as string)?.adapter; | |
| console.log('[LexicalState] 🔧 Adapter found?', !!adapter); | |
| if (!adapter) { | |
| console.log('[LexicalState] 🚀 Calling operation directly (no adapter)'); | |
| // Call operation directly without adapter (static operation) | |
| const { listAvailableBlocksOperation } = | |
| await import('../tools/operations/listAvailableBlocks'); | |
| console.log('[LexicalState] 📥 Operation imported, executing...'); | |
| const result = await listAvailableBlocksOperation.execute( | |
| { type: 'all' }, | |
| { documentId: 'static', executor: null as any }, | |
| ); | |
| console.log('[LexicalState] ✅ Operation result:', result); | |
| return result; | |
| } | |
| console.log('[LexicalState] 🔗 Delegating to adapter'); | |
| const result = await adapter.listAvailableBlocks(); | |
| console.log('[LexicalState] ✅ Adapter result:', result); | |
| // Delegate to adapter (following consistent pattern with all other operations) | |
| const params = typeof id === 'object' ? id : { id }; | |
| // Special case: this operation is static and doesn't require a document | |
| // If no document is found, call the operation directly | |
| const adapter = get().lexicals.get(params.id as string)?.adapter; | |
| if (!adapter) { | |
| // Call operation directly without adapter (static operation) | |
| const { listAvailableBlocksOperation } = | |
| await import('../tools/operations/listAvailableBlocks'); | |
| const result = await listAvailableBlocksOperation.execute( | |
| { type: 'all' }, | |
| { documentId: 'static', executor: null as any }, | |
| ); | |
| return result; | |
| } | |
| const result = await adapter.listAvailableBlocks(); |
| metadata: z.preprocess(val => { | ||
| // Convert empty strings to undefined (common LLM mistake) | ||
| if (val === '' || val === null) return undefined; | ||
|
|
||
| // If it's a string, try to parse it as JSON (LLMs often stringify metadata) | ||
| if (typeof val === 'string') { | ||
| try { | ||
| const parsed = JSON.parse(val); | ||
| // Only accept objects, not arrays or primitives | ||
| if (typeof parsed === 'object' && !Array.isArray(parsed)) { | ||
| return parsed; | ||
| } | ||
| return undefined; | ||
| } catch { | ||
| // Not valid JSON, convert to undefined | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| // Convert non-object values to undefined | ||
| if (typeof val !== 'object' || Array.isArray(val)) return undefined; | ||
| return val; | ||
| }, z.record(z.string(), z.unknown()).optional().describe('New block metadata (aligned with Jupyter format)')), |
There was a problem hiding this comment.
The metadata preprocessing logic is duplicated between insertBlock.ts and updateBlock.ts. Consider extracting this into a shared utility function to follow the DRY principle and ensure consistent behavior across all schema files. This would make future maintenance easier and reduce the risk of divergent behavior if changes are needed.
| // Send document-close for removed nodes | ||
| // TEMPORARILY DISABLED - Debug why traverse fails to find nodes | ||
| // const currentUuids = new Set(currentNodes.keys()); | ||
| // for (const trackedUuid of trackedNodesRef.current) { | ||
| // if (!currentUuids.has(trackedUuid)) { | ||
| // onDocumentCloseRef.current?.(trackedUuid); | ||
| // trackedNodesRef.current.delete(trackedUuid); | ||
| // } | ||
| // } | ||
| }; |
There was a problem hiding this comment.
The comment on lines 176-183 indicates that document-close messages are "TEMPORARILY DISABLED" for debugging purposes. This suggests incomplete work or a known issue. Before merging, either remove this code entirely, fix the underlying issue causing traverse to fail, or add a TODO with a tracking issue number. Leaving temporarily disabled code in production creates technical debt and confusion.
| // TEMPORARILY DISABLED - Testing if close messages are causing issues | ||
| // for (const uuid of trackedNodesRef.current) { | ||
| // onDocumentCloseRef.current?.(uuid); | ||
| // } |
There was a problem hiding this comment.
The comment on lines 202-205 indicates that close callbacks are "TEMPORARILY DISABLED" for testing. This is another instance of disabled code that should be resolved before merging. Either enable the functionality with proper fixes, remove it entirely, or add a TODO with a tracking issue. Shipping code with temporary debugging flags creates maintenance burden.
| // TEMPORARILY DISABLED - Testing if close messages are causing issues | |
| // for (const uuid of trackedNodesRef.current) { | |
| // onDocumentCloseRef.current?.(uuid); | |
| // } | |
| for (const uuid of trackedNodesRef.current) { | |
| onDocumentCloseRef.current?.(uuid); | |
| } |
2 participants
Fixes #421
Fixes datalayer/vscode-datalayer#149
Fixes datalayer/vscode-datalayer#151
Fixes datalayer/vscode-datalayer#111
Progress bar and spacing in output cell
The progress bar is now always visible to avoid having things appear and disappear which turned out to be very odd looking. Reduced the space as much as possible, and now if the cell is connected to a kernel, it will not make the space change or jump, the space is the same, if connected, or not
Update available blocks and improve tools