refactor: Group Add input block buttons in toolbar#287
Conversation
📝 WalkthroughWalkthroughConsolidates multiple specialized input-block commands into a single Sequence DiagramsequenceDiagram
actor User
participant CommandListener as DeepnoteNotebookCommandListener
participant QuickPick as Quick Pick UI
participant Editor as Notebook Editor
User->>CommandListener: Trigger deepnote.addInputBlock
CommandListener->>CommandListener: addInputBlockThroughPicker()
CommandListener->>CommandListener: Validate active notebook editor
CommandListener->>QuickPick: Show INPUT_BLOCK_TYPES picker
User->>QuickPick: Select input type (e.g., text, slider)
QuickPick->>CommandListener: Return selected type
CommandListener->>CommandListener: addInputBlock(selectedType)
CommandListener->>Editor: Insert input block cell
CommandListener->>Editor: Reveal and start edit on new cell
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 449-459: The QuickPick items array (items) for input block types
lacks description fields and should mirror the text block picker by adding
descriptive strings; update each object in the items array inside
deepnoteNotebookCommandListener (where items: (QuickPickItem & { inputBlockType:
InputBlockType })[] is defined) to include a description property using
l10n.t(...) (e.g., short localized descriptions like "Single-line text input",
"Multi-line textarea", "Choose from options", etc.) so each QuickPickItem has
label, description, and inputBlockType for consistent UX.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/common/constants.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
🔇 Additional comments (6)
src/platform/common/constants.ts (1)
239-239: LGTM!Constant placement and naming align with existing conventions.
package.nls.json (1)
269-269: LGTM!Localization key and value follow established patterns.
package.json (2)
233-238: LGTM!Command definition follows established patterns. Icon choice
$(symbol-parameter)appropriately represents input parameters.
1031-1037: LGTM!Toolbar consolidation reduces clutter. Group reordering is logical.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
191-193: LGTM!Command binding follows established pattern.
443-472: Fix incorrect placeholder text.Line 462: The placeholder says "Select an environment for this notebook" but should describe input block selection. Copy/paste error from
addTextBlockThroughPicker.🐛 Proposed fix
const selected = await window.showQuickPick(items, { - placeHolder: l10n.t('Select an environment for this notebook') + placeHolder: l10n.t('Select an input block type') });Likely an incorrect or invalid review comment.
…nds' into tomaskislan/group-add-input-block-commands-toolbar
… add description to input block picker items
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 446-487: The public method addInputBlockThroughPicker is out of
alphabetical order; move its declaration so it appears before addTextBlock (and
reorder other public methods as needed) to maintain alphabetical ordering of
public methods in the class (reorder the entire class's public methods to follow
alphabetical ordering).
- Around line 476-478: In the showQuickPick call that assigns const selected in
addInputBlockThroughPicker (the block starting with "const selected = await
window.showQuickPick(items, { placeHolder: l10n.t('Select an input block type')
});"), add the same QuickPick search options used in addTextBlockThroughPicker —
specifically set matchOnDescription: true and matchOnDetail: true — so the items
array can be filtered by description and detail; keep the existing placeHolder
and any other options intact.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
🔇 Additional comments (2)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
40-52: LGTM: Clean refactor to constant array.Centralizing input block types improves maintainability while preserving type safety. Mirrors the
TEXT_BLOCK_TYPESpattern.
194-196: LGTM: Command registration follows established pattern.Consistent with the
Commands.AddTextBlockregistration below.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Line 446: The public method addInputBlockThroughPicker is out of alphabetical
order relative to other public methods (it should appear before
addTextBlockCommandHandler); reorder the method declarations so public methods
follow alphabetical ordering — specifically move addInputBlockThroughPicker to
the position before addTextBlockCommandHandler among the class's public methods
to comply with the coding guideline.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
⏰ 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). (3)
- GitHub Check: Build & Package Extension
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
🔇 Additional comments (3)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
40-52: LGTM! Type derivation from const tuple is solid.The pattern ensures runtime values and types stay synchronized. Matches the existing
TEXT_BLOCK_TYPESapproach.
194-196: LGTM! Command registration is correct.Properly routes to the picker method and follows the established pattern.
446-489: LGTM! Implementation follows established patterns.The method correctly:
- Uses
l10n.t()for all user-facing strings- Validates active editor before proceeding
- Leverages
logger.info()for diagnostics- Handles cancellation properly
- Delegates to the existing
addInputBlock()method
Summary by CodeRabbit
New Features
UI Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.