Skip to content

Conversation

@andyjakubowski
Copy link
Contributor

@andyjakubowski andyjakubowski commented Oct 14, 2025

Hooks into the frontend file contents request lifecycle, and calls a custom API route to get .deepnote file contents.

Previously, we subclassed the backend ContentsManager to intercept and respond to requests for .deepnote file contents. To make our custom subclass work, users had to point their JupyterLab instances at our config file like:

jupyter lab --config="$(pwd)/jupyter-config/server-config/jupyter_server_config.json"

Requiring users to explicitly point at config files inside an extension installed via PyPI was not going to work, and this PR solves the problem.

Summary by CodeRabbit

  • Refactor

    • The extension no longer replaces the server’s contents manager; Jupyter’s default file handling is used.
  • New Features

    • Added a server API route that returns file data with ISO-normalized timestamps and structured error responses.
    • Client fetches .deepnote files from that API, validates responses, and converts .deepnote YAML into notebook content on open.
  • Documentation

    • README wording clarified for running the app and optional debug logging.
  • Chores

    • Simplified client request typing and related internals.

This doesn’t seem to do anything: `server_app.contents_manager = DeepnoteContentsManager(parent=server_app)`. What does seem to work is adding this field to a config file, and pointing `jupyterlab` at this config: `"contents_manager_class": "jupyterlab_deepnote.contents.DeepnoteContentsManager"`

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
@linear
Copy link

linear bot commented Oct 14, 2025

GRN-4926 Load backend extension config without explicit CLI flag

We’ve been testing the extension in JupyterLab like:

jupyter lab --config="$(pwd)/jupyter-config/server-config/jupyter_server_config.json"

We obviously can’t expect JupyterLab users to specify a config file for an extension they install from PyPI (jupyterlab-deepnote). They should be able to run jupyter lab and our extension should just work once it’s been installed.

@andyjakubowski andyjakubowski changed the title Delete noop assignment fix: load backend extension without explicit CLI flag Oct 14, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Removed the backend DeepnoteContentsManager class and its registration/wiring (deleted the class and the server config entry that set ServerApp.contents_manager). Extension init now only registers HTTP handlers and logging via setup_handlers; it no longer assigns a public contents manager during server extension load. Added a new async server route at /jupyterlab-deepnote/file that uses server.contents_manager via ensure_async to fetch file models (type="file", format="text", content=True), maps filesystem errors to HTTP 400/403/404/500 JSON responses, and returns the model under "deepnoteFileModel" with created/last_modified converted to ISO strings. Frontend DeepnoteContentProvider now calls that API using requestAPI (return type changed to Promise), validates responses with deepnoteFileResponseSchema, transforms YAML to notebook content, and returns a rebuilt Contents.IModel for .deepnote files; non-.deepnote paths fall back to the base provider.

Sequence Diagram(s)

sequenceDiagram
    participant Init as Extension Init
    participant Server as Jupyter Server

    rect #F8F9FB
    Init->>Server: setup_handlers(register HTTP routes)
    note right of Server: No DeepnoteContentsManager\nNo ServerApp.contents_manager assignment
    end
Loading
sequenceDiagram
    participant Route as Route Handler (async)
    participant ServerCM as server.contents_manager
    participant FileStore as Filesystem
    participant Client as Frontend

    Route->>ServerCM: ensure_async.get(path, type="file", format="text", content=True)
    rect #F3FFF0
    ServerCM->>FileStore: read file
    FileStore-->>ServerCM: text + metadata (created, last_modified)
    ServerCM-->>Route: file model
    Route->>Route: normalize datetimes to ISO
    Route-->>Client: JSON { "deepnoteFileModel": model }
    end
Loading
sequenceDiagram
    participant Frontend as DeepnoteContentProvider
    participant API as /jupyterlab-deepnote/file
    participant Local as Base Contents Provider

    Frontend->>Frontend: isDeepnoteFile(path.toLowerCase().endsWith(".deepnote"))?
    alt not .deepnote
        Frontend->>Local: return super.get(path)
    else .deepnote
        Frontend->>API: requestAPI({path})  %% returns Promise<unknown>
        API-->>Frontend: { deepnoteFileModel: modelData }
        Frontend->>Frontend: validate deepnoteFileResponseSchema
        Frontend->>Frontend: transformDeepnoteYamlToNotebookContent(modelData.content)
        Frontend-->>Local: return rebuilt Contents.IModel (type: notebook)
    end
Loading

Possibly related PRs

Suggested reviewers

  • saltenasl
  • jamesbhobbs
  • Artmann

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the main enhancement of the pull request by describing the addition of a custom API route for handling .deepnote files and aligns with the changes to both backend and frontend logic. It is concise, specific, and focuses on the primary feature without extraneous details.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 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 16222c2 and 63b3519.

📒 Files selected for processing (1)
  • jupyterlab_deepnote/handlers.py (1 hunks)

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: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f654bc and feaabc5.

📒 Files selected for processing (3)
  • jupyterlab_deepnote/__init__.py (1 hunks)
  • jupyterlab_deepnote/handlers.py (1 hunks)
  • src/deepnote-content-provider.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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/** : Deepnote integration code resides under src/notebooks/deepnote/
🧬 Code graph analysis (2)
jupyterlab_deepnote/__init__.py (1)
jupyterlab_deepnote/contents.py (1)
  • DeepnoteContentsManager (8-40)
jupyterlab_deepnote/handlers.py (1)
src/deepnote-content-provider.ts (1)
  • get (19-62)
🪛 Ruff (0.14.0)
jupyterlab_deepnote/handlers.py

17-17: Comment contains ambiguous (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?

(RUF003)

🔇 Additional comments (3)
jupyterlab_deepnote/__init__.py (1)

34-34: LGTM! Standard Jupyter extension pattern.

Setting contents_manager_class is the correct approach for registering a custom ContentsManager in server extensions. This enables automatic loading without explicit config.

src/deepnote-content-provider.ts (1)

23-27: Resolve endpoint integration verified. Handler reads the path query argument and returns the model and path in JSON.

jupyterlab_deepnote/handlers.py (1)

14-22: Verified contents_manager configuration. server_app.contents_manager_class = DeepnoteContentsManager in __init__.py ensures self.contents_manager in RouteHandler.get is a DeepnoteContentsManager instance.

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
@andyjakubowski andyjakubowski force-pushed the andy/grn-4926-load-backend-extension-config-without-explicit-cli-flag branch from 38025af to 87d62f6 Compare October 15, 2025 08:36
@andyjakubowski andyjakubowski changed the title fix: load backend extension without explicit CLI flag feat: add custom API route for .deepnote files Oct 15, 2025
@andyjakubowski andyjakubowski marked this pull request as ready for review October 15, 2025 08:42
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: 2

♻️ Duplicate comments (1)
jupyterlab_deepnote/handlers.py (1)

17-17: Fix ambiguous apostrophe character.

Use grave accent (`) instead of right single quotation mark (').

Apply this diff:

-        # Use Jupyter Server's contents_manager, not direct filesystem access.
+        # Use Jupyter Server`s contents_manager, not direct filesystem access.

Based on static analysis.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between feaabc5 and 87d62f6.

📒 Files selected for processing (6)
  • README.md (1 hunks)
  • jupyter-config/server-config/jupyter_server_config.json (0 hunks)
  • jupyterlab_deepnote/__init__.py (0 hunks)
  • jupyterlab_deepnote/contents.py (0 hunks)
  • jupyterlab_deepnote/handlers.py (1 hunks)
  • src/deepnote-content-provider.ts (1 hunks)
💤 Files with no reviewable changes (3)
  • jupyter-config/server-config/jupyter_server_config.json
  • jupyterlab_deepnote/init.py
  • jupyterlab_deepnote/contents.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T16:47:10.134Z
Learnt from: jamesbhobbs
PR: deepnote/jupyterlab-deepnote#11
File: .github/workflows/ci.yaml:27-29
Timestamp: 2025-10-14T16:47:10.134Z
Learning: In JupyterLab projects, `jlpm` is a wrapper around yarn that requires JupyterLab to be installed. The jupyterlab/maintainer-tools base-setup action provides yarn but not jlpm.

Applied to files:

  • README.md
🧬 Code graph analysis (2)
src/deepnote-content-provider.ts (2)
src/handler.ts (1)
  • requestAPI (12-46)
src/transform-deepnote-yaml-to-notebook-content.ts (1)
  • transformDeepnoteYamlToNotebookContent (6-56)
jupyterlab_deepnote/handlers.py (1)
src/deepnote-content-provider.ts (1)
  • get (7-41)
🪛 Ruff (0.14.0)
jupyterlab_deepnote/handlers.py

17-17: Comment contains ambiguous (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?

(RUF003)

🔇 Additional comments (3)
README.md (1)

117-123: Documentation correctly reflects the config-free workflow.

The changes remove references to the explicit --config flag and simplify the package manager guidance, aligning with the PR objective.

src/deepnote-content-provider.ts (1)

24-26: Transform error handling delegated to function.

The transformDeepnoteYamlToNotebookContent function already includes try-catch and throws descriptive errors, so explicit wrapping here may be redundant. Consider whether additional context is needed at this call site.

jupyterlab_deepnote/handlers.py (1)

18-20: No change needed: ensure_async is required
contents_manager.get is a synchronous call (not an async coroutine), so wrapping it with ensure_async is correct and should be kept.

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

♻️ Duplicate comments (1)
jupyterlab_deepnote/handlers.py (1)

17-17: Fix apostrophe character.

Use grave accent instead of right single quotation mark.

Apply this diff:

-        # Use Jupyter Server's contents_manager, not direct filesystem access.
+        # Use Jupyter Server's contents_manager, not direct filesystem access.

Based on static analysis.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between feaabc5 and 87d62f6.

📒 Files selected for processing (6)
  • README.md (1 hunks)
  • jupyter-config/server-config/jupyter_server_config.json (0 hunks)
  • jupyterlab_deepnote/__init__.py (0 hunks)
  • jupyterlab_deepnote/contents.py (0 hunks)
  • jupyterlab_deepnote/handlers.py (1 hunks)
  • src/deepnote-content-provider.ts (1 hunks)
💤 Files with no reviewable changes (3)
  • jupyterlab_deepnote/contents.py
  • jupyterlab_deepnote/init.py
  • jupyter-config/server-config/jupyter_server_config.json
🧰 Additional context used
🧠 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/deepnote-content-provider.ts
📚 Learning: 2025-10-14T16:47:10.134Z
Learnt from: jamesbhobbs
PR: deepnote/jupyterlab-deepnote#11
File: .github/workflows/ci.yaml:27-29
Timestamp: 2025-10-14T16:47:10.134Z
Learning: In JupyterLab projects, `jlpm` is a wrapper around yarn that requires JupyterLab to be installed. The jupyterlab/maintainer-tools base-setup action provides yarn but not jlpm.

Applied to files:

  • README.md
🧬 Code graph analysis (2)
src/deepnote-content-provider.ts (2)
src/handler.ts (1)
  • requestAPI (12-46)
src/transform-deepnote-yaml-to-notebook-content.ts (1)
  • transformDeepnoteYamlToNotebookContent (6-56)
jupyterlab_deepnote/handlers.py (1)
src/deepnote-content-provider.ts (1)
  • get (7-41)
🪛 Ruff (0.14.0)
jupyterlab_deepnote/handlers.py

17-17: Comment contains ambiguous (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?

(RUF003)

🔇 Additional comments (4)
README.md (1)

117-123: Documentation updates align with PR objectives.

The simplified setup instructions correctly reflect the removal of the explicit --config requirement. Clear and accurate.

jupyterlab_deepnote/handlers.py (2)

1-1: LGTM on new imports.

datetime and ensure_async are both used appropriately in the handler.

Also applies to: 6-6


36-36: Route name aligns with frontend usage.

The route path change from "get-example" to "file" matches the frontend's requestAPI call.

src/deepnote-content-provider.ts (1)

13-17: LGTM on non-deepnote path handling.

Early return for non-deepnote files is clean and correct.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@andyjakubowski andyjakubowski force-pushed the andy/grn-4926-load-backend-extension-config-without-explicit-cli-flag branch from 9c1485b to 689036e Compare October 15, 2025 08:48
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: 2

♻️ Duplicate comments (1)
src/deepnote-content-provider.ts (1)

20-20: Consider adding type safety for API response.

Using any loses compile-time type safety. Past review suggested defining a DeepnoteFileResponse interface.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87d62f6 and 689036e.

📒 Files selected for processing (1)
  • src/deepnote-content-provider.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/deepnote-content-provider.ts (2)
src/handler.ts (1)
  • requestAPI (12-46)
src/transform-deepnote-yaml-to-notebook-content.ts (1)
  • transformDeepnoteYamlToNotebookContent (6-56)
🔇 Additional comments (2)
src/deepnote-content-provider.ts (2)

11-17: LGTM!

Non-deepnote file handling is clean and correct.


31-35: LGTM!

Response validation is appropriate.

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
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.

🚀

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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 689036e and 83879a8.

📒 Files selected for processing (1)
  • jupyterlab_deepnote/handlers.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jupyterlab_deepnote/handlers.py (1)
src/deepnote-content-provider.ts (1)
  • get (7-57)
🪛 Ruff (0.14.0)
jupyterlab_deepnote/handlers.py

35-35: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)


40-40: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)


45-45: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
@andyjakubowski
Copy link
Contributor Author

@coderabbitai can you have another look, please? I addressed your change requests.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

@andyjakubowski, I'll review the changes now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

@andyjakubowski, I'll review the updated changes now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 83879a8 and 16222c2.

📒 Files selected for processing (2)
  • src/deepnote-content-provider.ts (1 hunks)
  • src/handler.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/deepnote-content-provider.ts (2)
src/handler.ts (1)
  • requestAPI (12-46)
src/transform-deepnote-yaml-to-notebook-content.ts (1)
  • transformDeepnoteYamlToNotebookContent (6-56)
🔇 Additional comments (2)
src/deepnote-content-provider.ts (1)

24-30: LGTM: case‑insensitive check and clean fallback

The .deepnote gate + super.get fallback is clear and correct.

src/handler.ts (1)

12-15: Add default generic to requestAPI to preserve call-site typing

Switch to a generic with default unknown so callers can specify the response type when needed.

-export async function requestAPI(
+export async function requestAPI<T = unknown>(
   endPoint = '',
   init: RequestInit = {}
-): Promise<unknown> {
+): Promise<T> {
@@
-  return data;
+  return data as T;
}

Comment on lines +8 to 17
const deepnoteFileResponseSchema = z.object({
deepnoteFileModel: z.object({
name: z.string(),
path: z.string(),
created: z.string(),
last_modified: z.string(),
content: z.string(),
mimetype: z.string().optional()
})
});
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Tighten schema (reject unknowns; validate timestamps)

Make both objects strict and validate ISO‑ish timestamps to fail early on bad data.

-const deepnoteFileResponseSchema = z.object({
-  deepnoteFileModel: z.object({
+const deepnoteFileResponseSchema = z.object({
+  deepnoteFileModel: z
+    .object({
       name: z.string(),
       path: z.string(),
-    created: z.string(),
-    last_modified: z.string(),
+      created: z.string().refine(v => !Number.isNaN(Date.parse(v)), 'invalid datetime'),
+      last_modified: z.string().refine(v => !Number.isNaN(Date.parse(v)), 'invalid datetime'),
       content: z.string(),
       mimetype: z.string().optional()
-  })
-});
+    })
+    .strict()
+}).strict();
📝 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.

Suggested change
const deepnoteFileResponseSchema = z.object({
deepnoteFileModel: z.object({
name: z.string(),
path: z.string(),
created: z.string(),
last_modified: z.string(),
content: z.string(),
mimetype: z.string().optional()
})
});
const deepnoteFileResponseSchema = z.object({
deepnoteFileModel: z
.object({
name: z.string(),
path: z.string(),
created: z.string().refine(v => !Number.isNaN(Date.parse(v)), 'invalid datetime'),
last_modified: z.string().refine(v => !Number.isNaN(Date.parse(v)), 'invalid datetime'),
content: z.string(),
mimetype: z.string().optional()
})
.strict()
}).strict();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that’s way too intense. What we have is sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andyjakubowski, understood. I'll keep the validation as-is.


✏️ Learnings added
Learnt from: andyjakubowski
PR: deepnote/jupyterlab-deepnote#27
File: src/deepnote-content-provider.ts:8-17
Timestamp: 2025-10-15T09:33:59.400Z
Learning: In the jupyterlab-deepnote project, basic Zod schema validation without `.strict()` mode or datetime refinement is preferred for API response validation in TypeScript files.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines +33 to +34
const data = await requestAPI(`file?path=${encodeURIComponent(localPath)}`);
const parsed = deepnoteFileResponseSchema.safeParse(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Prefer URLSearchParams for query composition

Safer and clearer than manual string interpolation/encoding.

-const data = await requestAPI(`file?path=${encodeURIComponent(localPath)}`);
+const params = new URLSearchParams({ path: localPath });
+const data = await requestAPI(`file?${params.toString()}`);
📝 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.

Suggested change
const data = await requestAPI(`file?path=${encodeURIComponent(localPath)}`);
const parsed = deepnoteFileResponseSchema.safeParse(data);
const params = new URLSearchParams({ path: localPath });
const data = await requestAPI(`file?${params.toString()}`);
const parsed = deepnoteFileResponseSchema.safeParse(data);
🤖 Prompt for AI Agents
In src/deepnote-content-provider.ts around lines 33 to 34, the current query is
composed via manual string interpolation and encodeURIComponent; replace that
with URLSearchParams to build the query safely and clearly: create a
URLSearchParams instance with the path parameter, use its toString() when
calling requestAPI (e.g. requestAPI(`file?${params.toString()}`)), and remove
the manual encodeURIComponent call so the query encoding is handled by
URLSearchParams.

Comment on lines +35 to 38
if (!parsed.success) {
console.error('Invalid API response shape', parsed.error);
throw new Error('Invalid API response shape');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Include path in error to aid debugging

Surface which file failed and keep the zod error in logs.

-if (!parsed.success) {
-  console.error('Invalid API response shape', parsed.error);
-  throw new Error('Invalid API response shape');
-}
+if (!parsed.success) {
+  const msg = `Invalid API response shape for ${localPath}`;
+  console.error(msg, parsed.error);
+  throw new Error(msg);
+}
📝 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.

Suggested change
if (!parsed.success) {
console.error('Invalid API response shape', parsed.error);
throw new Error('Invalid API response shape');
}
if (!parsed.success) {
const msg = `Invalid API response shape for ${localPath}`;
console.error(msg, parsed.error);
throw new Error(msg);
}
🤖 Prompt for AI Agents
In src/deepnote-content-provider.ts around lines 35 to 38, the error handling
for a failed Zod parse should include the path of the file that failed and
preserve the Zod error details; update the console.error call to log a clear
message containing the filePath (or relevant identifier) plus the full
parsed.error object, and throw a new Error that includes the file path (and
optionally a short summary) so callers can see which file failed while detailed
Zod info remains in the logs.

Comment on lines 41 to 56
// Transform the Deepnote YAML to Jupyter notebook content
const transformedModelContent =
await transformDeepnoteYamlToNotebookContent(
validatedModelContent.data.metadata.deepnote.rawYamlString
);

const transformedModel = {
...model,
content: transformedModelContent
const notebookContent = await transformDeepnoteYamlToNotebookContent(
modelData.content
);

const model: Contents.IModel = {
name: modelData.name,
path: modelData.path,
type: 'notebook',
writable: false,
created: modelData.created,
last_modified: modelData.last_modified,
mimetype: 'application/x-ipynb+json',
format: 'json',
content: notebookContent
};
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Respect options.content to avoid unnecessary heavy transforms

Skip YAML→notebook conversion and omit content/mime/format when content isn’t requested.

-// Transform the Deepnote YAML to Jupyter notebook content
-const notebookContent = await transformDeepnoteYamlToNotebookContent(
-  modelData.content
-);
+// Respect options?.content === false to avoid heavy transform
+const wantContent = options?.content !== false;
+const notebookContent = wantContent
+  ? await transformDeepnoteYamlToNotebookContent(modelData.content)
+  : null;
@@
-  mimetype: 'application/x-ipynb+json',
-  format: 'json',
+  mimetype: wantContent ? 'application/x-ipynb+json' : null,
+  format: wantContent ? 'json' : null,
   content: notebookContent
📝 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.

Suggested change
// Transform the Deepnote YAML to Jupyter notebook content
const transformedModelContent =
await transformDeepnoteYamlToNotebookContent(
validatedModelContent.data.metadata.deepnote.rawYamlString
);
const transformedModel = {
...model,
content: transformedModelContent
const notebookContent = await transformDeepnoteYamlToNotebookContent(
modelData.content
);
const model: Contents.IModel = {
name: modelData.name,
path: modelData.path,
type: 'notebook',
writable: false,
created: modelData.created,
last_modified: modelData.last_modified,
mimetype: 'application/x-ipynb+json',
format: 'json',
content: notebookContent
};
// Respect options?.content === false to avoid heavy transform
const wantContent = options?.content !== false;
const notebookContent = wantContent
? await transformDeepnoteYamlToNotebookContent(modelData.content)
: null;
const model: Contents.IModel = {
name: modelData.name,
path: modelData.path,
type: 'notebook',
writable: false,
created: modelData.created,
last_modified: modelData.last_modified,
mimetype: wantContent ? 'application/x-ipynb+json' : null,
format: wantContent ? 'json' : null,
content: notebookContent
};
🤖 Prompt for AI Agents
In src/deepnote-content-provider.ts around lines 41 to 56, the code always
transforms Deepnote YAML to notebook JSON and always sets
content/mimetype/format; change it to first check options?.content (or the
equivalent Jupyter Contents API options flag) and only run
transformDeepnoteYamlToNotebookContent when content is requested. If
options.content is falsy, omit the content property from the returned model and
do not set mimetype or format (or set them to undefined), and avoid calling the
heavy transform function; if content is requested, perform the transform and
populate content, mimetype ('application/x-ipynb+json') and format ('json') as
before.

@andyjakubowski andyjakubowski merged commit 3346ed4 into devin/1760105835-add-lint-staged Oct 15, 2025
5 checks passed
andyjakubowski added a commit that referenced this pull request Oct 15, 2025
* Delete noop assignment

This doesn’t seem to do anything: `server_app.contents_manager = DeepnoteContentsManager(parent=server_app)`. What does seem to work is adding this field to a config file, and pointing `jupyterlab` at this config: `"contents_manager_class": "jupyterlab_deepnote.contents.DeepnoteContentsManager"`

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

* Add custom API route for .deepnote files

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

* Update src/deepnote-content-provider.ts

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Add error handling

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

* Validate API response against Zod schema

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

* Lowercase path in .deepnote file check

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

* Drop unused exception aliases

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>

---------

Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@coderabbitai coderabbitai bot mentioned this pull request Oct 16, 2025
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