Skip to content

Conversation

@tkislan
Copy link
Member

@tkislan tkislan commented Oct 30, 2025

image

Summary by CodeRabbit

  • New Features

    • Added error boundary for dataframe rendering with detailed error messages and stack traces for debugging.
  • Refactor

    • Standardized SQL integration identifier usage across the codebase for consistency and maintainability.
    • Improved dataframe rendering pipeline with enhanced error handling and logging.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
…erer

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

📝 Walkthrough

Walkthrough

The PR consolidates the SQL integration identifier by replacing hard-coded strings with a centralized constant across notebook and test files. It refactors the webview-side dataframe rendering pipeline by introducing a new DataframeRendererContainer component that extracts and passes metadata to the underlying renderer, and adds an ErrorBoundary with ErrorFallback for error handling. The dispose function parameter is also renamed for consistency.

Sequence Diagram(s)

sequenceDiagram
    participant index as index.ts
    participant container as DataframeRendererContainer
    participant renderer as DataframeRenderer
    participant boundary as ErrorBoundary

    Note over index,boundary: New Flow

    index->>boundary: Render with outputJson,<br/>outputMetadata
    boundary->>container: Props passed through
    container->>container: Extract cellId &<br/>dataFrameMetadata
    container->>renderer: Render with extracted<br/>metadata & data
    renderer-->>container: Success
    container-->>boundary: Rendered output
    
    alt Error occurs
        renderer-->>boundary: Error thrown
        boundary->>boundary: Catch error
        boundary->>boundary: Render ErrorFallback<br/>(message + stack)
    end
Loading

Possibly related PRs

  • feat: Copy and export data frames. #70: Modifies DataframeRenderer to add copy/export UI; this PR refactors the rendering pipeline with a container and error boundary.
  • fix: Dataframe SQL block support #118: Replaces hard-coded 'deepnote-dataframe-sql' with DATAFRAME_SQL_INTEGRATION_ID constant in SQL integration handling.
  • deepnote/deepnote-internal#18286: Adopts the shared DATAFRAME_SQL_INTEGRATION_ID constant in notebook-agent code, aligning with this PR's constant centralization.

Suggested reviewers

  • saltenasl
  • jamesbhobbs
  • jankuca

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: Fix dataframe output duplicate rendering" directly relates to the changeset. The PR refactors the dataframe rendering pipeline by introducing a DataframeRendererContainer and ErrorBoundary to replace inline rendering logic, which addresses rendering issues. While the title could be more specific about the architectural approach (container pattern, error handling), it accurately conveys that the PR targets a dataframe rendering problem and avoids generic or misleading phrasing.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 065b4c0 and 7382903.

📒 Files selected for processing (6)
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2 hunks)
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (2 hunks)
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2 hunks)
  • src/webviews/webview-side/dataframe-renderer/DataframeRendererContainer.tsx (1 hunks)
  • src/webviews/webview-side/dataframe-renderer/ErrorBoundary.tsx (1 hunks)
  • src/webviews/webview-side/dataframe-renderer/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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/webviews/webview-side/dataframe-renderer/ErrorBoundary.tsx
  • src/webviews/webview-side/dataframe-renderer/DataframeRendererContainer.tsx
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/webviews/webview-side/dataframe-renderer/index.ts
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/webviews/webview-side/dataframe-renderer/index.ts
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.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/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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 then property to avoid hanging tests

Files:

  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
🧬 Code graph analysis (4)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
  • DATAFRAME_SQL_INTEGRATION_ID (5-5)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
  • DATAFRAME_SQL_INTEGRATION_ID (5-5)
src/webviews/webview-side/dataframe-renderer/index.ts (2)
src/webviews/webview-side/dataframe-renderer/ErrorBoundary.tsx (1)
  • ErrorFallback (4-37)
src/webviews/webview-side/dataframe-renderer/DataframeRendererContainer.tsx (2)
  • DataframeRendererContainer (14-34)
  • Metadata (6-12)
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
  • DATAFRAME_SQL_INTEGRATION_ID (5-5)
⏰ 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 & Package Extension
  • GitHub Check: Build & Test
🔇 Additional comments (10)
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (2)

24-24: LGTM: Centralized constant eliminates magic string.


738-738: LGTM: Constant usage in test aligns with centralized definition.

src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2)

12-12: LGTM: Import properly grouped with related types.


78-78: LGTM: Test data now references single source of truth.

src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)

33-33: LGTM: Centralizes SQL integration identifier.


193-193: LGTM: Production code now uses centralized constant.

src/webviews/webview-side/dataframe-renderer/ErrorBoundary.tsx (1)

17-21: Localize the fallback copy

All user-facing strings must flow through l10n.t(). Please wrap “Error rendering dataframe” and “Stack trace” (and any related text) with localized resources instead of hardcoded literals. [coding_guidelines]

+import { l10n } from '@vscode/l10n';-            <div style={{ fontWeight: 'bold', marginBottom: '8px' }}>Error rendering dataframe</div>
+            <div style={{ fontWeight: 'bold', marginBottom: '8px' }}>
+                {l10n.t('dataframe.errorRendering', 'Error rendering dataframe')}
+            </div>-                <summary>Stack trace</summary>
+                <summary>{l10n.t('dataframe.stackTrace', 'Stack trace')}</summary>

As per coding guidelines.

src/webviews/webview-side/dataframe-renderer/DataframeRendererContainer.tsx (1)

24-33: Use ILogger instead of console.log

The new container logs through console.log, which violates the “use ILogger” requirement. Please route these traces through the shared logging service (e.g., inject ILogger or reuse an existing logger) rather than writing directly to the console. [coding_guidelines]

src/webviews/webview-side/dataframe-renderer/index.ts (2)

20-22: Route errors through ILogger

Please replace the console.error call in the boundary’s onError handler with the project’s ILogger service so errors are captured by the standard telemetry pipeline. [coding_guidelines]


34-44: Fix global dispose to unmount real roots

When id is null, this loop walks document.children, which is just the <html> element. Consequently none of the rendered output containers are ever unmounted, so their React trees linger and leak memory. Iterate over the actual host containers (e.g., document.body.children or a tracked set of mounts) before calling ReactDOM.unmountComponentAtNode.

-            if (id == null) {
-                for (let i = 0; i < document.children.length; i++) {
-                    const child = document.children.item(i);
-                    if (child == null) {
-                        continue;
-                    }
-                    ReactDOM.unmountComponentAtNode(child);
-                }
-                return;
-            }
+            if (id == null) {
+                const root = document.body;
+                if (!root) {
+                    return;
+                }
+                for (const child of Array.from(root.children)) {
+                    ReactDOM.unmountComponentAtNode(child);
+                }
+                return;
+            }

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72%. Comparing base (065b4c0) to head (7382903).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #136   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        541     541           
  Lines      41481   41483    +2     
  Branches    5027    5027           
=====================================
+ Hits       30209   30211    +2     
  Misses      9593    9593           
  Partials    1679    1679           
Files with missing lines Coverage Δ
...ebooks/deepnote/deepnoteNotebookCommandListener.ts 83% <100%> (+<1%) ⬆️
...pnote/deepnoteNotebookCommandListener.unit.test.ts 98% <100%> (+<1%) ⬆️
...tegrationEnvironmentVariablesProvider.unit.test.ts 99% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tkislan tkislan marked this pull request as ready for review October 30, 2025 09:02
@tkislan tkislan requested a review from a team as a code owner October 30, 2025 09:02
Copy link
Member

@Artmann Artmann left a comment

Choose a reason for hiding this comment

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

Awesome!

@tkislan tkislan merged commit 87e8346 into main Oct 30, 2025
12 of 13 checks passed
@tkislan tkislan deleted the tk/fix-dataframe-renderer branch October 30, 2025 12:38
dinohamzic pushed a commit that referenced this pull request Oct 30, 2025
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
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.

3 participants