Skip to content

Conversation

@jankuca
Copy link
Contributor

@jankuca jankuca commented Oct 21, 2025

Resolves GRN-4964

Before:

Screenshot 2025-10-17 at 13 04 26

After:

Screenshot 2025-10-21 at 12 51 57

Even though SQL caching is not available locally (we cannot upload to S3), we can still renderer these outputs. Some imported .deepnote files may contain them, and the SQL blocks do generate them.

Summary by CodeRabbit

  • New Features
    • Added a SQL Metadata Renderer to display query status, cache creation timestamps, and human-readable sizes with colored status indicators.
  • Integration
    • Registered the renderer so SQL metadata outputs are recognized and rendered in notebooks.
  • Chores
    • Added build configuration to produce the renderer bundle.
  • Tests
    • Added unit tests ensuring SQL metadata survives round-trip conversions and renders intact.

@linear
Copy link

linear bot commented Oct 21, 2025

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

📝 Walkthrough

Walkthrough

Adds a Deepnote SQL metadata notebook renderer: a memoized React component that displays SQL metadata (status, optional cache timestamp, size), a VS Code renderer activation module that mounts/unmounts the component for outputs with MIME application/vnd.deepnote.sql-output-metadata+json, an esbuild build step producing the renderer bundle, package.json registration for the new renderer entry, and bidirectional conversion plus unit tests in deepnoteDataConverter to preserve this MIME when translating between Blocks and VS Code NotebookCell outputs.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Notebook
    participant Renderer as "VSCode Renderer (activate)"
    participant ReactDOM
    participant Component as "SqlMetadataRenderer"

    Notebook->>Renderer: NotebookCellOutputItem (mime: application/vnd.deepnote.sql-output-metadata+json)
    activate Renderer
    Renderer->>Renderer: outputItem.json() -> data
    Renderer->>ReactDOM: create root element & render(<SqlMetadataRenderer data={data} />)
    activate ReactDOM
    ReactDOM->>Component: mount with data
    activate Component
    Component->>Component: compute statusInfo, format cache_created_at & size_in_bytes
    Component-->>ReactDOM: rendered DOM
    deactivate Component
    ReactDOM-->>Renderer: render complete
    deactivate ReactDOM
    Renderer->>Notebook: rendered output displayed
    deactivate Renderer
Loading

Possibly related PRs

Suggested reviewers

  • andyjakubowski
  • jamesbhobbs
  • Artmann

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Linked Issues Check ❓ Inconclusive The linked issue GRN-4964 contains no specific coding requirements to validate against. While the PR description states the objective is to "add a renderer for SQL metadata outputs," and the implementation clearly accomplishes this (new renderer component, mime type support, data conversion, tests, build integration), the absence of explicit requirements in the linked issue prevents definitive compliance verification. Provide detailed requirements from GRN-4964 to validate that all coding objectives are met. The PR appears aligned with its stated goal, but explicit issue requirements are needed to confirm compliance.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat: SQL metadata output renderer" directly reflects the primary change across all files. All modifications—from build configuration to component implementation to data conversion—center on adding this renderer. The title is specific, concise, and immediately communicates the feature to someone reviewing history.
Out of Scope Changes Check ✅ Passed All changes remain tightly scoped to implementing the SQL metadata renderer: build configuration (build.ts), registration (package.json), data conversion support (deepnoteDataConverter.ts), component implementation (SqlMetadataRenderer.tsx), VSCode integration (index.ts), and comprehensive tests. No extraneous modifications or unrelated features are present.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 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 2dcacce and 18324ff.

📒 Files selected for processing (1)
  • package.json (1 hunks)
⏰ 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: Lint & Format
  • GitHub Check: Build & Test
  • GitHub Check: Build & Package Extension
🔇 Additional comments (1)
package.json (1)

1860-1868: Build pipeline correctly configured; entry is production-ready.

The build step in build.ts explicitly handles the SQL metadata renderer, outputting to the exact entrypoint specified in package.json. Source file exists and the build configuration is correct.


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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 83b7ab9 and e1e2d82.

📒 Files selected for processing (6)
  • build/esbuild/build.ts (1 hunks)
  • package.json (1 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.ts (2 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (2 hunks)
  • src/webviews/webview-side/sql-metadata-renderer/SqlMetadataRenderer.tsx (1 hunks)
  • src/webviews/webview-side/sql-metadata-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/sql-metadata-renderer/SqlMetadataRenderer.tsx
  • src/webviews/webview-side/sql-metadata-renderer/index.ts
  • build/esbuild/build.ts
  • src/notebooks/deepnote/deepnoteDataConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.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/webviews/webview-side/sql-metadata-renderer/index.ts
  • build/esbuild/build.ts
  • src/notebooks/deepnote/deepnoteDataConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
src/notebooks/deepnote/deepnoteDataConverter.ts

📄 CodeRabbit inference engine (CLAUDE.md)

deepnoteDataConverter.ts performs Deepnote data transformations

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.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/deepnoteDataConverter.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/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
🧠 Learnings (2)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations

Applied to files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format

Applied to files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (2)
src/webviews/webview-side/sql-metadata-renderer/index.ts (1)
src/webviews/webview-side/sql-metadata-renderer/SqlMetadataRenderer.tsx (1)
  • SqlMetadataRenderer (14-96)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • NotebookCellOutputItem (16-69)
🔇 Additional comments (6)
build/esbuild/build.ts (1)

339-350: LGTM - Follows established pattern.

The build configuration mirrors the existing renderer setup (vega-renderer) correctly.

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

223-227: LGTM - Consistent with existing MIME type handling.

The SQL metadata MIME type handling follows the same pattern as dataframe and vega outputs.


312-319: LGTM - Symmetric conversion.

The reverse transformation correctly reconstructs the SQL metadata output item.

src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (2)

435-475: LGTM - Thorough test coverage.

The test validates MIME type registration and JSON payload preservation correctly.


513-558: LGTM - Round-trip validation.

Confirms SQL metadata survives blocks→cells→blocks conversion without data loss.

package.json (1)

1860-1868: LGTM - Renderer registration correct.

Configuration matches existing renderer pattern and aligns with build output path.

Base automatically changed from jk/feat/sql-block-credentials to main October 21, 2025 13:25
@jankuca jankuca force-pushed the jk/feat/sql-metadata-renderer branch from e1e2d82 to 85cfd9e Compare October 21, 2025 15:18
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71%. Comparing base (a626bb3) to head (18324ff).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/notebooks/deepnote/deepnoteDataConverter.ts 75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #100   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        527     527           
  Lines      39474   39500   +26     
  Branches    4933    4935    +2     
=====================================
+ Hits       28277   28312   +35     
+ Misses      9563    9547   -16     
- Partials    1634    1641    +7     
Files with missing lines Coverage Δ
...ebooks/deepnote/deepnoteDataConverter.unit.test.ts 100% <100%> (ø)
src/notebooks/deepnote/deepnoteDataConverter.ts 57% <75%> (+6%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/webviews/webview-side/sql-metadata-renderer/index.ts (1)

26-26: Replace console.error with ILogger.

Coding guidelines require using the ILogger service instead of console methods.

As per coding guidelines

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85cfd9e and 2dcacce.

📒 Files selected for processing (2)
  • src/webviews/webview-side/sql-metadata-renderer/SqlMetadataRenderer.tsx (1 hunks)
  • src/webviews/webview-side/sql-metadata-renderer/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/sql-metadata-renderer/SqlMetadataRenderer.tsx
  • src/webviews/webview-side/sql-metadata-renderer/index.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/webviews/webview-side/sql-metadata-renderer/index.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`

Applied to files:

  • src/webviews/webview-side/sql-metadata-renderer/index.ts
🧬 Code graph analysis (1)
src/webviews/webview-side/sql-metadata-renderer/index.ts (1)
src/webviews/webview-side/sql-metadata-renderer/SqlMetadataRenderer.tsx (1)
  • SqlMetadataRenderer (56-96)
⏰ 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: Lint & Format
🔇 Additional comments (3)
src/webviews/webview-side/sql-metadata-renderer/index.ts (1)

13-13: LGTM: Proper cleanup implemented.

The roots Map and disposeOutputItem implementation correctly prevent memory leaks by unmounting React components.

Also applies to: 22-22, 35-43

src/webviews/webview-side/sql-metadata-renderer/SqlMetadataRenderer.tsx (2)

14-41: LGTM: Helper functions properly extracted.

Moving getStatusMessage outside the component avoids recreation on each render.


83-86: Date formatting is locale-aware.

Using toLocaleString() correctly respects user locale preferences, which aligns with VS Code UX expectations.

@jankuca jankuca marked this pull request as ready for review October 22, 2025 13:55
@jankuca jankuca merged commit 9c81863 into main Oct 22, 2025
12 checks passed
@jankuca jankuca deleted the jk/feat/sql-metadata-renderer branch October 22, 2025 15:18
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