fix: Don't open the init notebook by default. #133
Conversation
📝 WalkthroughWalkthroughDeserialization/serialization now treat DeepnoteFile as the source of truth instead of DeepnoteProject. YAML is parsed into a DeepnoteFile; the code throws if the file’s Sequence DiagramsequenceDiagram
participant Client
participant Deserializer
participant Validator
participant Selector
participant Storage
Client->>Deserializer: Submit YAML payload
Deserializer->>Deserializer: Parse YAML → DeepnoteFile
Deserializer->>Validator: Ensure deepnoteFile.notebooks not empty
Validator-->>Deserializer: OK / throw error
alt explicit notebook_id provided
Deserializer->>Selector: Use provided notebook_id
Selector-->>Deserializer: Resolved notebook
else no notebook_id
Deserializer->>Selector: findDefaultNotebook(deepnoteFile)
note right of Selector #E8F0FE: Prefer non‑Init by name\nFallback to first when only Init exists
Selector-->>Deserializer: Resolved notebook
end
Deserializer->>Storage: Persist metadata (project.id, name, version, notebook_id)
Storage-->>Deserializer: Stored
Deserializer-->>Client: Return serialized result (DeepnoteFile-derived)
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteSerializer.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/deepnoteSerializer.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/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteFile(6-6)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build & Package Extension
- GitHub Check: Spell Check
- GitHub Check: Build & Test
- GitHub Check: Audit - All
- GitHub Check: Audit - Production
- GitHub Check: Check Licenses
- GitHub Check: TypeCheck
- GitHub Check: Lint & Format
🔇 Additional comments (2)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
47-95: LGTM: Deserialization updated correctly.The type change to
DeepnoteFileand updated references are consistent. The empty notebooks validation and default notebook selection logic properly support the PR objective.
117-161: No issues found. The review comment is incorrect.DeepnoteProject is a type alias for DeepnoteFile (
export type DeepnoteProject = DeepnoteFile;). Line 74 stores DeepnoteFile, and line 117 retrieves it as DeepnoteProject—these are the same type. The metadata property exists on both. The cast is redundant but harmless, and line 144 will execute without error.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
=====================================
Coverage 72% 72%
=====================================
Files 541 541
Lines 41443 41481 +38
Branches 5023 5027 +4
=====================================
+ Hits 30174 30209 +35
- Misses 9591 9593 +2
- Partials 1678 1679 +1
🚀 New features to boost your workflow:
|
|
@copilot Can you add a test for this change? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
41-43: Message nit: say “Deserialization cancelled”.Minor mismatch; this is the deserialize path.
- throw new Error('Serialization cancelled'); + throw new Error(l10n.t('Deserialization cancelled'));
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
186-190: Complete JSDoc for param and return.- /** - * Finds the default notebook to open when no selection is made. - * @param file - * @returns - */ + /** + * Finds the default notebook to open when no selection is made. + * @param file The Deepnote file containing project and notebook data + * @returns The default notebook to open, prioritizing non-init notebooks + */Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteSerializer.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/deepnoteSerializer.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/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteFile(6-6)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Check Licenses
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
- GitHub Check: TypeCheck
- GitHub Check: Audit - All
- GitHub Check: Audit - Production
- GitHub Check: Spell Check
- GitHub Check: Build & Package Extension
🔇 Additional comments (3)
src/notebooks/deepnote/deepnoteSerializer.ts (3)
63-65: Default-selection logic looks good.Uses deepnoteFile and helper to avoid opening Init by default. LGTM.
80-85: Metadata sourcing from DeepnoteFile: LGTM.
74-75: Review comment is incorrect; no type safety issue exists.
DeepnoteProjectis defined as a deprecated alias forDeepnoteFile(deepnoteTypes.ts:12). They are identical types, so storing aDeepnoteFileand retrieving it asDeepnoteProjectintroduces no shape mismatch or risk. The code is type-safe as written.A consistency refactor to prefer
DeepnoteFilethroughout (aligning with the deprecation notice) would improve clarity, but this is a style concern, not a correctness issue.Likely an incorrect or invalid review comment.
* Initial plan * test: Add tests for default notebook selection logic Co-authored-by: Artmann <91954+Artmann@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Artmann <91954+Artmann@users.noreply.github.com>
| const nameA = a.name.toLowerCase(); | ||
| const nameB = b.name.toLowerCase(); | ||
|
|
||
| // If there's an Init notebook, we don't want to open it by default unless it's the only one. | ||
| if (file.project.initNotebookId && a.id === file.project.initNotebookId) { | ||
| return 1; | ||
| } | ||
|
|
||
| if (file.project.initNotebookId && b.id === file.project.initNotebookId) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (nameA < nameB) { |
There was a problem hiding this comment.
It's not the most readable logic tbh. It might be better to exclude the init notebook from the list, sort the list, look at the first item, and then fall back to the init notebook when there is no first item.
BTW on the web, we are not considering the Init notebook the "default notebook" at all and we do not open it automatically even when there is no other content in the project.
There was a problem hiding this comment.
Sure, I hate sorting code and never understand it, so that sounds better.
This happens mostly when you click a file from the file explorer, so then I guess we want to show the Init notebook.
| const yamlContent = ` | ||
| version: 1.0 | ||
| metadata: |
There was a problem hiding this comment.
What about making sure this string is actually a valid document but defining it here as parsed and serializing it to yaml before invoking the test?
| /** | ||
| * Helper function to convert a DeepnoteProject object with version to YAML format | ||
| */ | ||
| function projectToYaml(projectData: { version: string; metadata: any; project: any }): Uint8Array { |
There was a problem hiding this comment.
can we use the actual type of the project format? (if there is one 😅 )
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
186-190: Complete JSDoc descriptions.Missing parameter and return value descriptions.
Apply this diff:
/** * Finds the default notebook to open when no selection is made. - * @param file - * @returns + * @param file The Deepnote file containing project and notebook data + * @returns The default notebook to open, prioritizing non-init notebooks, or undefined if no notebooks exist */
47-61: Guard yaml.load result and localize all error messages consistently.
yaml.loadcan return undefined or non-object values. The cast at line 47 is unsafe. Additionally, error messages at lines 50 and 59 are not localized, while line 67 usesl10n.t().Apply this diff:
- const deepnoteFile = yaml.load(contentString) as DeepnoteFile; + const parsed = yaml.load(contentString); - if (!deepnoteFile.project?.notebooks) { - throw new Error('Invalid Deepnote file: no notebooks found'); + if ( + !parsed || + typeof parsed !== 'object' || + !('project' in parsed) || + !parsed.project || + typeof parsed.project !== 'object' || + !('notebooks' in parsed.project) || + !Array.isArray(parsed.project.notebooks) + ) { + throw new Error(l10n.t('Invalid Deepnote file: no notebooks found')); } + + const deepnoteFile = parsed as DeepnoteFile; const projectId = deepnoteFile.project.id; const notebookId = this.findCurrentNotebookId(projectId); logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${notebookId}`); if (deepnoteFile.project.notebooks.length === 0) { - throw new Error('Deepnote project contains no notebooks.'); + throw new Error(l10n.t('Deepnote project contains no notebooks.')); }As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteSerializer.ts(6 hunks)src/notebooks/deepnote/deepnoteSerializer.unit.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.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/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.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/deepnoteSerializer.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/deepnoteSerializer.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/platform/deepnote/deepnoteTypes.ts (2)
DeepnoteFile(6-6)DeepnoteNotebook(18-18)
⏰ 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 (9)
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (6)
2-2: LGTM!Import added for YAML serialization in tests.
63-69: LGTM!Helper cleanly serializes test data to YAML format.
326-378: LGTM!Test correctly validates that non-init notebooks are preferred. The "Main" notebook is selected over "Init" when both are present.
380-417: LGTM!Test correctly validates Init as the fallback when it's the only notebook.
419-483: LGTM!Test correctly validates alphabetical ordering when no initNotebookId is provided. "Alpha Notebook" is selected from ["Zebra", "Alpha", "Bravo"].
485-550: LGTM!Test correctly validates Init deprioritization. "Alpha" is selected over "Init" even though "Init" would come after "Alpha" alphabetically when considering case-insensitive sorting.
src/notebooks/deepnote/deepnoteSerializer.ts (3)
8-8: LGTM!DeepnoteNotebook type imported for explicit typing.
74-84: LGTM!Metadata correctly derived from DeepnoteFile structure.
117-138: LGTM!Serialization correctly uses DeepnoteFile type throughout.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.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/deepnoteSerializer.unit.test.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/notebooks/deepnote/deepnoteSerializer.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/notebooks/deepnote/deepnoteSerializer.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/deepnoteSerializer.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteFile(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
- GitHub Check: Build & Package Extension
🔇 Additional comments (6)
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (6)
2-2: LGTM: Imports support the new test helpers.The
js-yamlandDeepnoteFiletype are correctly imported for YAML serialization and type safety.Also applies to: 7-7
327-378: LGTM: Test correctly validates Init notebook is skipped.The test properly verifies that when multiple notebooks exist, the Init notebook is not selected by default.
380-417: LGTM: Edge case for single Init notebook handled.Correctly verifies that Init is selected when it's the only available notebook.
419-483: LGTM: Alphabetical selection verified.Test correctly validates that the alphabetically first notebook is selected when no initNotebookId is specified.
485-550: LGTM: Init sorting behavior confirmed.Test properly verifies that Init notebook is treated as last in sort order, with the comment on line 547 clarifying the expected behavior.
326-551: Test suite comprehensively covers default notebook selection.The four test cases properly validate the new behavior: Init skipped when alternatives exist, Init as fallback when alone, alphabetical selection, and Init sorted last. This addresses the comment requesting tests for this change.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests