feat: Better big number block UI #135
Conversation
📝 WalkthroughWalkthroughAdds a Big Number Comparison Settings feature for Deepnote notebooks: a VS Code notebook status bar provider for big-number cells, a webview provider and React webview for configuring comparison settings, new types and localization strings, updates to converters/tests to use valueExpression-based Python cells, and service registry registrations to activate the status bar provider. Also adds an esbuild build step for the webview bundle. Public APIs added: Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant StatusBar as DeepnoteBigNumberCellStatusBarProvider
participant Provider as BigNumberComparisonSettingsWebviewProvider
participant Panel as Webview Panel (React)
participant Notebook as Notebook Cell Metadata
User->>StatusBar: Click "Configure Comparison"
activate StatusBar
StatusBar->>Provider: show(cell, token)
activate Provider
Provider->>Panel: create webview panel + inject script
activate Panel
Panel->>Notebook: read deepnote_big_number_* metadata
Notebook-->>Panel: return settings (or defaults)
Panel->>Provider: postReady/init complete
Panel->>Panel: wait for locInit, render UI
User->>Panel: edit settings, click Save
Panel->>Provider: postMessage(type: 'save', settings)
Provider->>Notebook: apply WorkspaceEdit / NotebookEdit.updateCellMetadata
Notebook-->>Provider: apply result / error
Provider-->>StatusBar: resolve with new settings
StatusBar->>StatusBar: refresh status bar items
deactivate Panel
deactivate Provider
deactivate StatusBar
alt User cancels
User->>Panel: click Cancel
Panel->>Provider: postMessage(type: 'cancel')
Provider-->>StatusBar: resolve null
end
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
build/esbuild/build.ts(1 hunks)src/messageTypes.ts(1 hunks)src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts(1 hunks)src/notebooks/deepnote/converters/chartBigNumberBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts(5 hunks)src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/platform/common/utils/localize.ts(1 hunks)src/platform/notebooks/deepnote/types.ts(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/bigNumberComparisonSettings.css(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/index.tsx(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/notebooks/deepnote/types.tssrc/webviews/webview-side/bigNumberComparisonSettings/types.tssrc/messageTypes.tsbuild/esbuild/build.tssrc/notebooks/deepnote/bigNumberComparisonSettingsWebview.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.tssrc/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.tssrc/platform/common/utils/localize.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/platform/notebooks/deepnote/types.tssrc/webviews/webview-side/bigNumberComparisonSettings/types.tssrc/messageTypes.tsbuild/esbuild/build.tssrc/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsxsrc/notebooks/serviceRegistry.web.tssrc/notebooks/deepnote/bigNumberComparisonSettingsWebview.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.tssrc/platform/common/utils/localize.tssrc/webviews/webview-side/bigNumberComparisonSettings/index.tsx
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/notebooks/deepnote/types.tssrc/platform/common/utils/localize.ts
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/notebooks/serviceRegistry.web.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/bigNumberComparisonSettingsWebview.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.tssrc/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.ts
🧬 Code graph analysis (7)
src/platform/notebooks/deepnote/types.ts (1)
src/webviews/webview-side/bigNumberComparisonSettings/types.ts (1)
BigNumberComparisonSettings(1-7)
src/webviews/webview-side/bigNumberComparisonSettings/types.ts (1)
src/platform/notebooks/deepnote/types.ts (1)
BigNumberComparisonSettings(29-35)
src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx (4)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/platform/notebooks/deepnote/types.ts (1)
BigNumberComparisonSettings(29-35)src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts (1)
handleMessage(184-215)src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)src/notebooks/deepnote/converters/constants.ts (1)
DEEPNOTE_VSCODE_RAW_CONTENT_KEY(1-1)
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.ts (3)
src/notebooks/deepnote/converters/constants.ts (1)
DEEPNOTE_VSCODE_RAW_CONTENT_KEY(1-1)src/notebooks/deepnote/deepnoteSchemas.ts (1)
DeepnoteBigNumberMetadataSchema(11-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts (1)
src/platform/deepnote/pocket.ts (1)
src/webviews/webview-side/bigNumberComparisonSettings/index.tsx (2)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx (1)
BigNumberComparisonSettingsPanel(11-215)
| private readonly comparisonSettingsWebview: BigNumberComparisonSettingsWebviewProvider; | ||
|
|
||
| public readonly onDidChangeCellStatusBarItems = this._onDidChangeCellStatusBarItems.event; | ||
|
|
||
| constructor(@inject(IExtensionContext) extensionContext: IExtensionContext) { | ||
| this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inject the webview provider instead of instantiating it.
The coding guidelines mandate injecting interfaces rather than concrete classes. Directly instantiating BigNumberComparisonSettingsWebviewProvider violates this principle and prevents testing and future flexibility.
Based on coding guidelines.
Define an interface for the webview provider (e.g., IBigNumberComparisonSettingsWebviewProvider) and inject it via the constructor:
+import { IBigNumberComparisonSettingsWebviewProvider } from './bigNumberComparisonSettingsWebview';
+
@injectable()
export class DeepnoteBigNumberCellStatusBarProvider
implements NotebookCellStatusBarItemProvider, IExtensionSyncActivationService
{
private readonly disposables: Disposable[] = [];
private readonly _onDidChangeCellStatusBarItems = new EventEmitter<void>();
- private readonly comparisonSettingsWebview: BigNumberComparisonSettingsWebviewProvider;
+ private readonly comparisonSettingsWebview: IBigNumberComparisonSettingsWebviewProvider;
public readonly onDidChangeCellStatusBarItems = this._onDidChangeCellStatusBarItems.event;
- constructor(@inject(IExtensionContext) extensionContext: IExtensionContext) {
- this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext);
+ constructor(
+ @inject(IExtensionContext) extensionContext: IExtensionContext,
+ @inject(IBigNumberComparisonSettingsWebviewProvider) comparisonSettingsWebview: IBigNumberComparisonSettingsWebviewProvider
+ ) {
+ this.comparisonSettingsWebview = comparisonSettingsWebview;
}You'll also need to register the webview provider interface in the service registry.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
36 to 42, the class directly constructs
BigNumberComparisonSettingsWebviewProvider instead of depending on an injected
interface; change the private field to the interface type (e.g.,
IBigNumberComparisonSettingsWebviewProvider), remove the "new" instantiation,
and accept that interface via the constructor (annotated for DI). Add or update
the import for the interface, update all call sites to use the injected
interface, and register/bind IBigNumberComparisonSettingsWebviewProvider to
BigNumberComparisonSettingsWebviewProvider in the service registry/container so
tests can mock the interface.
| constructor(@inject(IExtensionContext) extensionContext: IExtensionContext) { | ||
| this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add ILogger for error tracking.
The coding guidelines require using the ILogger service. Error conditions (lines 76, 95, 114, 303) should be logged for debugging, not just shown to users.
Based on coding guidelines.
Inject ILogger and log errors:
+import { ILogger } from '../../platform/logging/types';
+
@injectable()
export class DeepnoteBigNumberCellStatusBarProvider
implements NotebookCellStatusBarItemProvider, IExtensionSyncActivationService
{
private readonly disposables: Disposable[] = [];
private readonly _onDidChangeCellStatusBarItems = new EventEmitter<void>();
private readonly comparisonSettingsWebview: BigNumberComparisonSettingsWebviewProvider;
public readonly onDidChangeCellStatusBarItems = this._onDidChangeCellStatusBarItems.event;
- constructor(@inject(IExtensionContext) extensionContext: IExtensionContext) {
+ constructor(
+ @inject(IExtensionContext) extensionContext: IExtensionContext,
+ @inject(ILogger) private readonly logger: ILogger
+ ) {
this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext);
}Then log errors where they occur, e.g.:
if (!cell) {
+ this.logger.error('No active notebook cell for updateBigNumberTitle command');
void window.showErrorMessage(l10n.t('No active notebook cell'));
return;
}📝 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.
| constructor(@inject(IExtensionContext) extensionContext: IExtensionContext) { | |
| this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext); | |
| } | |
| import { ILogger } from '../../platform/logging/types'; | |
| constructor( | |
| @inject(IExtensionContext) extensionContext: IExtensionContext, | |
| @inject(ILogger) private readonly logger: ILogger | |
| ) { | |
| this.comparisonSettingsWebview = new BigNumberComparisonSettingsWebviewProvider(extensionContext); | |
| } |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
40-42, inject the ILogger service into the constructor (add a private readonly
logger: ILogger parameter) and store it on the instance; then at the error
handling points noted (around lines 76, 95, 114, and 303) call logger.error with
the error/exception and contextual message in addition to the existing
user-facing notifications so errors are recorded for debugging (include error
stack/message and a short context string for each location).
| commands.registerCommand('deepnote.updateBigNumberTitle', async (cell?: NotebookCell) => { | ||
| if (!cell) { | ||
| const activeEditor = window.activeNotebookEditor; | ||
| if (activeEditor && activeEditor.selection) { | ||
| cell = activeEditor.notebook.cellAt(activeEditor.selection.start); | ||
| } | ||
| } | ||
|
|
||
| if (!cell) { | ||
| void window.showErrorMessage(l10n.t('No active notebook cell')); | ||
| return; | ||
| } | ||
|
|
||
| await this.updateTitle(cell); | ||
| }) | ||
| ); | ||
|
|
||
| // Command to update big number format | ||
| this.disposables.push( | ||
| commands.registerCommand('deepnote.updateBigNumberFormat', async (cell?: NotebookCell) => { | ||
| if (!cell) { | ||
| const activeEditor = window.activeNotebookEditor; | ||
| if (activeEditor && activeEditor.selection) { | ||
| cell = activeEditor.notebook.cellAt(activeEditor.selection.start); | ||
| } | ||
| } | ||
|
|
||
| if (!cell) { | ||
| void window.showErrorMessage(l10n.t('No active notebook cell')); | ||
| return; | ||
| } | ||
|
|
||
| await this.updateFormat(cell); | ||
| }) | ||
| ); | ||
|
|
||
| // Command to configure comparison settings | ||
| this.disposables.push( | ||
| commands.registerCommand('deepnote.configureBigNumberComparison', async (cell?: NotebookCell) => { | ||
| if (!cell) { | ||
| const activeEditor = window.activeNotebookEditor; | ||
| if (activeEditor && activeEditor.selection) { | ||
| cell = activeEditor.notebook.cellAt(activeEditor.selection.start); | ||
| } | ||
| } | ||
|
|
||
| if (!cell) { | ||
| void window.showErrorMessage(l10n.t('No active notebook cell')); | ||
| return; | ||
| } | ||
|
|
||
| await this.configureComparison(cell); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicate cell resolution logic.
All three command handlers repeat identical logic to resolve the active cell when none is provided.
Extract to a helper method:
+ private getTargetCell(providedCell?: NotebookCell): NotebookCell | undefined {
+ if (providedCell) {
+ return providedCell;
+ }
+
+ const activeEditor = window.activeNotebookEditor;
+ if (activeEditor && activeEditor.selection) {
+ return activeEditor.notebook.cellAt(activeEditor.selection.start);
+ }
+
+ return undefined;
+ }
+
private registerCommands(): void {
// Command to update big number title
this.disposables.push(
commands.registerCommand('deepnote.updateBigNumberTitle', async (cell?: NotebookCell) => {
- if (!cell) {
- const activeEditor = window.activeNotebookEditor;
- if (activeEditor && activeEditor.selection) {
- cell = activeEditor.notebook.cellAt(activeEditor.selection.start);
- }
- }
+ cell = this.getTargetCell(cell);
if (!cell) {
void window.showErrorMessage(l10n.t('No active notebook cell'));
return;
}
await this.updateTitle(cell);
})
);Apply the same pattern to the other two command handlers.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
67-120, the three command handlers duplicate the same logic to resolve the
active notebook cell when no cell argument is provided; extract that logic into
a single private helper method on the class (e.g.,
getActiveOrSelectedCell(cell?: NotebookCell): NotebookCell | undefined) that
returns the resolved cell or undefined, update each command to call this helper
and bail out with the existing showErrorMessage when it returns undefined, and
replace the repeated block in all three handlers with a single call to the
helper to keep behavior identical.
| private async configureComparison(cell: NotebookCell): Promise<void> { | ||
| const settings = await this.comparisonSettingsWebview.show(cell); | ||
| if (settings) { | ||
| this._onDidChangeCellStatusBarItems.fire(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pass cancellation token to webview provider.
The configureComparison method doesn't accept or pass a CancellationToken to the webview provider's show() method. Since opening a webview can be a long-running operation, cancellation support should be added.
Add cancellation token parameter:
- private async configureComparison(cell: NotebookCell): Promise<void> {
- const settings = await this.comparisonSettingsWebview.show(cell);
+ private async configureComparison(cell: NotebookCell, token?: CancellationToken): Promise<void> {
+ const settings = await this.comparisonSettingsWebview.show(cell, token);
if (settings) {
this._onDidChangeCellStatusBarItems.fire();
}
}Note: You'll also need to update the command registration to pass the token if available.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
285 to 290, the configureComparison method calls
comparisonSettingsWebview.show() without a CancellationToken; update the method
signature to accept a CancellationToken parameter and pass that token into
this.comparisonSettingsWebview.show(token), then keep the existing behavior of
firing _onDidChangeCellStatusBarItems when settings are returned; also update
any command/handler registration that calls configureComparison to forward the
CancellationToken from the command invocation (if provided) so webview opening
can be cancelled.
| if (!initialized) { | ||
| return <div className={`big-number-comparison-settings-panel theme-${baseTheme}`}>Loading...</div>; | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Localize the loading label.
Hardcoding “Loading...” bypasses our localization flow. Please fetch it through getLocString (adding a key if needed) so the spinner text respects locale.
🤖 Prompt for AI Agents
In
src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx
around lines 100–104 the component returns a hardcoded "Loading..." string;
replace that literal with the localized string via getLocString (e.g.
getLocString('bigNumberComparison.loading') or similar), add the new key/value
to the localization resource files, and ensure getLocString is
imported/available in this file so the spinner text respects locale.
jankuca
force-pushed
the
jk/big-number-ux
branch
from
35f5c21 to
7e72cf5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
=====================================
- Coverage 73% 72% -1%
=====================================
Files 548 548
Lines 43272 43342 +70
Branches 5256 5264 +8
=====================================
+ Hits 31590 31629 +39
- Misses 9942 9969 +27
- Partials 1740 1744 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.ts (1)
53-60: Legacy fallback still missing.The prior review comment about restoring legacy
block.contentfallback remains unaddressed. Older big-number blocks stored their value inblock.content, but the current code only reads from metadata. This will blank existing blocks that haven't been migrated.Apply the suggested fix from the previous review:
- const valueExpression = deepnoteBigNumberMetadataResult.success - ? deepnoteBigNumberMetadataResult.data.deepnote_big_number_value - : DEFAULT_BIG_NUMBER_CONFIG.deepnote_big_number_value; + const valueExpressionFromMetadata = deepnoteBigNumberMetadataResult.success + ? deepnoteBigNumberMetadataResult.data.deepnote_big_number_value + : DEFAULT_BIG_NUMBER_CONFIG.deepnote_big_number_value; + const legacyValueExpression = + typeof block.content === 'string' ? block.content.trim() : DEFAULT_BIG_NUMBER_CONFIG.deepnote_big_number_value; + const valueExpression = valueExpressionFromMetadata || legacyValueExpression;src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx (1)
100-102: Localize the loading state.This literal bypasses the localization flow; please surface it through
getLocString(add a key such asbigNumberComparisonLoading).- if (!initialized) { - return <div className={`big-number-comparison-settings-panel theme-${baseTheme}`}>Loading...</div>; - } + if (!initialized) { + return ( + <div className={`big-number-comparison-settings-panel theme-${baseTheme}`}> + {getLocString('bigNumberComparisonLoading', 'Loading…')} + </div> + ); + }src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts (7)
36-42: Still constructing provider directly.Previous feedback stands: instantiate
BigNumberComparisonSettingsWebviewProvidervia an injected interface instead ofnew. Please introduce an interface (e.g.,IBigNumberComparisonSettingsWebviewProvider) and inject it through the constructor so DI and testing stay intact. As per coding guidelines.
40-42: ILogger still missing.We still show user errors without logging. Inject
ILoggerin the constructor and record the failures at Lines 76, 95, 114, and 303 to keep telemetry. As per coding guidelines.
67-119: Extract cell-resolution helper.The three command handlers still repeat the same active-cell resolution logic. Please factor this into a private helper (e.g.,
getTargetCell) and reuse it to cut the duplication. As per coding guidelines.
140-205: Replace magic alignment numbers with the enum.
alignment: 1/2should useNotebookCellStatusBarAlignment.Left/Right. Add the enum import and swap the literals for readability and future safety. As per coding guidelines.
250-254: Clarify template variables in the prompt.The input prompt still references
{{var}}without telling users which placeholders are valid. Please update the prompt (or add validation) to list concrete tokens so people know what works. As per coding guidelines.
285-287: Wire cancellation through to the webview.
configureComparisonstill drops cancellation support. Accept aCancellationToken(from callers) and pass it intocomparisonSettingsWebview.showso a cancelled command tears down the panel cleanly. As per coding guidelines.
299-307: Log the failed metadata edit.When
workspace.applyEditfails we only toast the user. After injectingILogger, log the error message and cause so we keep diagnostics. As per coding guidelines.src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts (1)
139-148: Normalize legacy comparison types.
sendInitialDatastill forwardsdeepnote_big_number_comparison_typeverbatim. Legacy blocks emit'absolute-change', which the webview doesn’t render, leaving the radios blank. Map'absolute-change'to'absolute-value'before posting the settings so migrated notebooks work. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit 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 (15)
build/esbuild/build.ts(1 hunks)src/messageTypes.ts(1 hunks)src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts(1 hunks)src/notebooks/deepnote/converters/chartBigNumberBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts(5 hunks)src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/deepnoteNotebookCommandListener.ts(2 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/platform/common/utils/localize.ts(1 hunks)src/platform/notebooks/deepnote/types.ts(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/bigNumberComparisonSettings.css(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/index.tsx(1 hunks)src/webviews/webview-side/bigNumberComparisonSettings/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.tssrc/messageTypes.tssrc/platform/common/utils/localize.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/webviews/webview-side/bigNumberComparisonSettings/types.tssrc/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/bigNumberComparisonSettingsWebview.tsbuild/esbuild/build.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.tssrc/webviews/webview-side/bigNumberComparisonSettings/index.tsxsrc/messageTypes.tssrc/platform/common/utils/localize.tssrc/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/webviews/webview-side/bigNumberComparisonSettings/types.tssrc/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.tssrc/platform/notebooks/deepnote/types.tssrc/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsxsrc/notebooks/deepnote/bigNumberComparisonSettingsWebview.tssrc/notebooks/serviceRegistry.node.tsbuild/esbuild/build.tssrc/notebooks/serviceRegistry.web.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/common/utils/localize.tssrc/platform/notebooks/deepnote/types.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.ts
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/notebooks/serviceRegistry.web.ts
🧬 Code graph analysis (9)
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.ts (3)
src/notebooks/deepnote/converters/constants.ts (1)
DEEPNOTE_VSCODE_RAW_CONTENT_KEY(1-1)src/notebooks/deepnote/deepnoteSchemas.ts (1)
DeepnoteBigNumberMetadataSchema(11-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
src/webviews/webview-side/bigNumberComparisonSettings/index.tsx (2)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx (1)
BigNumberComparisonSettingsPanel(11-215)
src/notebooks/deepnote/converters/chartBigNumberBlockConverter.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)src/notebooks/deepnote/converters/constants.ts (1)
DEEPNOTE_VSCODE_RAW_CONTENT_KEY(1-1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
src/kernels/execution/notebookUpdater.ts (1)
chainWithPendingUpdates(24-52)src/test/mocks/vsc/extHostedTypes.ts (3)
NotebookCellData(2546-2572)NotebookEdit(2467-2530)NotebookRange(2436-2466)
src/webviews/webview-side/bigNumberComparisonSettings/types.ts (1)
src/platform/notebooks/deepnote/types.ts (1)
BigNumberComparisonSettings(29-35)
src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts (3)
src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts (1)
injectable(28-314)src/platform/deepnote/pocket.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2467-2530)
src/platform/notebooks/deepnote/types.ts (1)
src/webviews/webview-side/bigNumberComparisonSettings/types.ts (1)
BigNumberComparisonSettings(1-7)
src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx (4)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/bigNumberComparisonSettings/types.ts (2)
BigNumberComparisonSettings(1-7)WebviewMessage(9-13)src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts (1)
handleMessage(184-215)src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
src/notebooks/deepnote/bigNumberComparisonSettingsWebview.ts (5)
src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts (1)
injectable(30-314)src/platform/notebooks/deepnote/types.ts (2)
BigNumberComparisonSettings(29-35)BigNumberComparisonWebviewMessage(40-44)src/platform/logging/index.ts (1)
logger(35-48)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2467-2530)src/platform/errors/types.ts (1)
WrappedError(23-57)
⏰ 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). (2)
- GitHub Check: Build & Test
- GitHub Check: copilot-setup-steps
🔇 Additional comments (9)
src/webviews/webview-side/bigNumberComparisonSettings/bigNumberComparisonSettings.css (1)
1-178: LGTM!CSS follows VS Code theming conventions and includes proper accessibility features.
src/platform/common/utils/localize.ts (1)
923-940: LGTM!Localization strings follow existing patterns and cover all UI text needs.
build/esbuild/build.ts (1)
387-392: LGTM!Build entry mirrors the selectInputSettings pattern correctly.
src/notebooks/serviceRegistry.node.ts (2)
71-71: LGTM!Import follows existing patterns.
187-190: LGTM!Service registration follows DI conventions correctly.
src/webviews/webview-side/bigNumberComparisonSettings/index.tsx (1)
1-20: LGTM!Webview bootstrap follows established patterns for VS Code webviews.
src/notebooks/serviceRegistry.web.ts (2)
53-53: LGTM!Import follows existing patterns.
121-124: LGTM!Service registration follows DI conventions correctly.
src/platform/notebooks/deepnote/types.ts (1)
26-44: Type definitions mirror webview-side types.BigNumberComparisonSettings and BigNumberComparisonWebviewMessage duplicate the definitions in
src/webviews/webview-side/bigNumberComparisonSettings/types.ts. This is consistent with the pattern used for SelectInputSettings but creates maintenance overhead if the types diverge.
| text: 'Big Number', | ||
| alignment: 1, // NotebookCellStatusBarAlignment.Left | ||
| priority: 100, | ||
| tooltip: this.buildTooltip(metadata) | ||
| }); | ||
|
|
||
| // 2. Title editor | ||
| const title = (metadata?.deepnote_big_number_title as string) || ''; | ||
| const titleText = title ? `$(edit) ${title}` : '$(edit) Set title'; | ||
| items.push({ | ||
| text: titleText, | ||
| alignment: 1, | ||
| priority: 95, | ||
| tooltip: l10n.t('Click to edit title'), | ||
| command: { | ||
| title: l10n.t('Edit Title'), | ||
| command: 'deepnote.updateBigNumberTitle', | ||
| arguments: [cell] | ||
| } | ||
| }); | ||
|
|
||
| // 3. Format selector | ||
| const format = (metadata?.deepnote_big_number_format as string) || 'number'; | ||
| const formatLabel = this.getFormatLabel(format); | ||
| items.push({ | ||
| text: formatLabel, | ||
| alignment: 1, | ||
| priority: 90, | ||
| tooltip: l10n.t('Click to change format'), | ||
| command: { | ||
| title: l10n.t('Change Format'), | ||
| command: 'deepnote.updateBigNumberFormat', | ||
| arguments: [cell] | ||
| } | ||
| }); | ||
|
|
||
| // 4. Comparison button | ||
| const comparisonEnabled = (metadata?.deepnote_big_number_comparison_enabled as boolean) ?? false; | ||
| const comparisonType = (metadata?.deepnote_big_number_comparison_type as string) || ''; | ||
| const comparisonValue = (metadata?.deepnote_big_number_comparison_value as string) || ''; | ||
|
|
||
| let comparisonText: string; | ||
| if (comparisonEnabled && comparisonType && comparisonValue) { | ||
| const comparisonTypeLabel = comparisonType === 'percentage-change' ? '% change' : 'vs'; | ||
| const comparisonTitle = (metadata?.deepnote_big_number_comparison_title as string) || ''; | ||
| comparisonText = `Comparison: ${comparisonTypeLabel} ${ | ||
| comparisonTitle ? `${comparisonTitle} (${comparisonValue})` : comparisonValue | ||
| }`; | ||
| } else { | ||
| comparisonText = 'Set up comparison'; | ||
| } | ||
|
|
||
| items.push({ | ||
| text: comparisonText, | ||
| alignment: 1, | ||
| priority: 85, | ||
| tooltip: l10n.t('Click to configure comparison'), | ||
| command: { | ||
| title: l10n.t('Configure Comparison'), | ||
| command: 'deepnote.configureBigNumberComparison', | ||
| arguments: [cell] | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize the status-bar strings.
User-visible strings like 'Big Number', '$(edit) Set title', 'Comparison: ...', and 'Set up comparison' bypass l10n.t. Please wrap all of them (including the tooltip text produced by comparisonTypeLabel) in localization calls so they appear in translations. As per coding guidelines.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
141 to 204, several user-facing strings are not localized: 'Big Number', the
title fallback '$(edit) Set title', formatLabel display, comparison-related
strings ('% change'/'vs', the comparisonText prefix "Comparison: ...", and 'Set
up comparison'), and any tooltip text derived from comparisonTypeLabel; wrap
each of these in l10n.t (use placeholder interpolation for dynamic pieces like
the title, comparison label/value, and format label) so they go through the
translation system, and update tooltip strings similarly; ensure you preserve VS
Code icon markers (e.g. '$(edit)') when passing to l10n.t and use descriptive
keys/messages suitable for translators.
| { label: 'Number', description: 'Display as a number', picked: currentFormat === 'number' }, | ||
| { label: 'Currency', description: 'Display as currency', picked: currentFormat === 'currency' }, | ||
| { label: 'Percent', description: 'Display as percentage', picked: currentFormat === 'percent' } | ||
| ]; | ||
|
|
||
| const selected = await window.showQuickPick(formatOptions, { | ||
| placeHolder: l10n.t('Select format for big number') | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize QuickPick labels/descriptions.
'Number', 'Currency', 'Percent', and their descriptions should flow through l10n.t. Please move both label and description text into localized strings. As per coding guidelines.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteBigNumberCellStatusBarProvider.ts around lines
268 to 275, the QuickPick item labels and descriptions are hard-coded ("Number",
"Currency", "Percent" and their descriptions); replace each literal with l10n.t
calls using appropriate localization keys (e.g.
l10n.t('bigNumber.format.number'), l10n.t('bigNumber.format.numberDescription'),
etc.), update all three items' label and description to use l10n.t, and ensure
the chosen keys are added to the localization resource files.
| commands.registerCommand('notebook.cell.insertCodeCellBelow', () => this.insertCodeCellBelowInterceptor()) | ||
| ); |
There was a problem hiding this comment.
Preserve command context when overriding the insert command.
Overriding notebook.cell.insertCodeCellBelow without forwarding the incoming arguments drops the context cell/editor that VS Code passes from cell toolbars, context menus, or other extensions. In Deepnote notebooks this now inserts below the wrong cell (or falls back without context), regressing the core command. Please accept ...args in the handler, reuse them when delegating to the default command, and honour the context cell when you compute insertIndex/language detection.
- this.disposableRegistry.push(
- commands.registerCommand('notebook.cell.insertCodeCellBelow', () => this.insertCodeCellBelowInterceptor())
- );
+ this.disposableRegistry.push(
+ commands.registerCommand('notebook.cell.insertCodeCellBelow', (...args) =>
+ this.insertCodeCellBelowInterceptor(...args)
+ )
+ );
...
- private async insertCodeCellBelowInterceptor(): Promise<void> {
+ private async insertCodeCellBelowInterceptor(...args: unknown[]): Promise<void> {
const editor = window.activeNotebookEditor;
if (!editor || editor.notebook.notebookType !== 'deepnote') {
- await commands.executeCommand('default:notebook.cell.insertCodeCellBelow');
+ await commands.executeCommand('default:notebook.cell.insertCodeCellBelow', ...args);
return;
}
- // Get the current selection
- const selection = editor.selection;
- if (!selection) {
- // No selection, use default behavior
- await commands.executeCommand('default:notebook.cell.insertCodeCellBelow');
+ const contextCell = this.getContextCellFromArgs(args);
+ const selection = editor.selection ?? (contextCell ? new NotebookRange(contextCell.index, contextCell.index + 1) : undefined);
+ if (!selection) {
+ await commands.executeCommand('default:notebook.cell.insertCodeCellBelow', ...args);
return;
}
- // Get the current cell to check its language
- const currentCell = editor.notebook.cellAt(selection.start);
+ const currentCell = contextCell ?? editor.notebook.cellAt(selection.start);
...
- const insertIndex = selection.end;
+ const insertIndex = contextCell ? contextCell.index + 1 : selection.end;
...
- await commands.executeCommand('default:notebook.cell.insertCodeCellBelow');
+ await commands.executeCommand('default:notebook.cell.insertCodeCellBelow', ...args);
}
}
+
+ private getContextCellFromArgs(args: unknown[]): NotebookCell | undefined {
+ const [first] = args;
+ if (!first || typeof first !== 'object') {
+ return undefined;
+ }
+ if ((first as NotebookCell).index !== undefined) {
+ return first as NotebookCell;
+ }
+ const candidate = first as { cell?: NotebookCell };
+ return candidate.cell;
+ }Also applies to: 382-425
| <option value="">Same as main value</option> | ||
| <option value="number">Number</option> | ||
| <option value="currency">Currency</option> | ||
| <option value="percent">Percent</option> | ||
| </select> | ||
| <div className="help-text"> | ||
| {getLocString( | ||
| 'comparisonFormatHelp', | ||
| 'Leave empty to use the same format as the main value' | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Run the select options through localization.
These option labels are user-facing but stay hardcoded, breaching our getLocString requirement. Please pipe them through localization (adding the corresponding keys alongside the other comparison strings).
- <option value="">Same as main value</option>
- <option value="number">Number</option>
- <option value="currency">Currency</option>
- <option value="percent">Percent</option>
+ <option value="">
+ {getLocString('comparisonFormatSameAsMain', 'Same as main value')}
+ </option>
+ <option value="number">{getLocString('comparisonFormatNumber', 'Number')}</option>
+ <option value="currency">
+ {getLocString('comparisonFormatCurrency', 'Currency')}
+ </option>
+ <option value="percent">{getLocString('comparisonFormatPercent', 'Percent')}</option>🤖 Prompt for AI Agents
In
src/webviews/webview-side/bigNumberComparisonSettings/BigNumberComparisonSettingsPanel.tsx
around lines 190 to 200 the option labels are hardcoded strings; replace each
visible label with calls to getLocString using new keys (e.g.
'comparisonFormatOptionSame', 'comparisonFormatOptionNumber',
'comparisonFormatOptionCurrency', 'comparisonFormatOptionPercent') while keeping
the option value attributes unchanged, and add those keys and translations to
the localization resource alongside the other comparison strings.
Summary by CodeRabbit