fix: Add ESLint rules for type safety and fix violations #46
Conversation
- Add @typescript-eslint/no-floating-promises: error - Add @typescript-eslint/no-non-null-assertion: error - Add @typescript-eslint/prefer-nullish-coalescing: error - Change @typescript-eslint/no-explicit-any from off to error Fixes: - Fix floating promise in NotebookPicker constructor by adding .catch() - Fix non-null assertion in NotebookPicker.onAfterAttach with null check - Fix prefer-nullish-coalescing in handler.ts by using ?? instead of || - Add inline eslint-disable comments for legitimate any usage in handler.ts
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
📝 WalkthroughWalkthroughThis PR tightens TypeScript linting by enabling @typescript-eslint rules in package.json: no-explicit-any, no-floating-promises, no-non-null-assertion, and prefer-nullish-coalescing (all as errors). NotebookPicker adds error handling for panel initialization (logs .catch) and guards a parent non-null assertion before sending ResizeMessage.UnknownSize. requestAPI/handler.ts normalizes non-TypeError errors, narrows response data typing to string|unknown, only JSON.parses non-empty strings, and extracts non-OK messages from data.message when present. NotebookPicker tests are refactored to use typed mocks/helpers. jest.config.js allows additional ES modules (@deepnote, yaml) to be transformed by Jest. Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as NotebookPicker UI
participant Panel as NotebookPanel (mockable)
participant Parent as ParentWidget
participant Logger as Console
rect #f8f9fa
Note over UI,Panel: Panel init now logs errors & guards parent access
UI->>Panel: fromJSON(payload)
Panel-->>UI: Promise (resolve/reject)
alt resolve
UI->>UI: render list
alt parent exists
UI->>Parent: send ResizeMessage.UnknownSize
end
else reject
UI->>Logger: console.error(error)
end
end
sequenceDiagram
autonumber
participant Client as requestAPI caller
participant Handler as requestAPI
participant Server as HTTP
rect #f8f9fa
Client->>Handler: requestAPI(endpoint, init)
Handler->>Server: fetch(...)
Server-->>Handler: Response (status, body)
Handler->>Handler: read body -> data (string | unknown)
alt data is non-empty string
Handler->>Handler: try JSON.parse(data) -> parsed
else
Handler->>Handler: keep data as-is
end
alt response ok
Handler-->>Client: return parsed/data
else
Handler->>Handler: msg = (isObject(data) && data.message) ? data.message : String(data)
Handler->>Client: throw ResponseError(msg) (wrap non-TypeError as TypeError)
end
end
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 selected for processing (3)
package.json(1 hunks)src/components/NotebookPicker.tsx(2 hunks)src/handler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.{ts,tsx} : Use strict type checking and prefer type safety over convenience
📚 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/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks
Applied to files:
src/components/NotebookPicker.tsx
🔇 Additional comments (4)
src/components/NotebookPicker.tsx (1)
76-78: Proper null guard.Correctly replaces non-null assertion with defensive check.
package.json (1)
164-164: Strong type safety rules enabled.The four rules enforce better TypeScript practices. All violations addressed in corresponding code changes.
Based on learnings.
Also applies to: 167-169
src/handler.ts (2)
32-33: Acceptableanyusage.The variable holds dynamic response data that starts as text and may be parsed as JSON. Type flexibility is justified here.
44-44: Verify intentionality of nullish coalescing change.The shift from
||to??changes behavior whendata.messageis an empty string,0, orfalse—these now become the error message instead of falling back to the fulldataobject. No tests found to validate this scenario. Confirm this was intentional and tested with error responses containing falsy but defined message values.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 50.20% 52.56% +2.36%
==========================================
Files 13 13
Lines 249 253 +4
Branches 26 28 +2
==========================================
+ Hits 125 133 +8
+ Misses 119 116 -3
+ Partials 5 4 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jamesbhobbs
changed the title
jamesbhobbs
changed the base branch from
main
to
devin/1761239214-add-test-coverage
jamesbhobbs
changed the base branch from
devin/1761239214-add-test-coverage
to
main
Improves test coverage for the .catch() error handler added to handle promise rejections in the constructor.
- Add test for null model in handleChange - Add test for invalid metadata in handleChange - Add test for onAfterAttach without parent - Import Message type for test Coverage improved from 84.09% to 97.72% for NotebookPicker.tsx
Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts (1)
1-328: Verify ESLint compliance for test file.Multiple
as anytype assertions throughout. Ensure test files are exempted from@typescript-eslint/no-explicit-anyor these will fail linting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts(1 hunks)src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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 : On conversion failures, provide detailed error reporting with suggested fixes
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts
🧬 Code graph analysis (1)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
⏰ 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: check_release
- GitHub Check: build
🔇 Additional comments (2)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
37-304: Comprehensive test coverage.The test suite thoroughly covers code cells, markdown cells, special block types, edge cases (truncated outputs, empty metadata), and defensive checks (non-mutation). Well-structured and follows best practices.
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts (1)
23-328: Excellent test coverage.Comprehensive suite covering transformation logic, edge cases (empty/missing notebooks), error handling, metadata preservation, and mixed content. Well-organized with clear test names.
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts
Outdated
Show resolved
Hide resolved
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts
Outdated
Show resolved
Hide resolved
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 (2)
jest.config.js(1 hunks)src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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 : On conversion failures, provide detailed error reporting with suggested fixes
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
📚 Learning: 2025-10-22T11:56:07.963Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-10-22T11:56:07.963Z
Learning: Applies to **/*.{spec,test}.{tsx,jsx} : Avoid excessive mocking in React component tests; use TestProvider for context; prefer apolloMocks and flags props over jest.mock for internal hooks/contexts
Applied to files:
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
🧬 Code graph analysis (1)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
🪛 GitHub Actions: Build
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
[warning] 1-1: Code style issues found in this file. Run 'Prettier --write' to fix.
🪛 GitHub Actions: CI
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
[warning] 1-1: Code style issues found in this file. Run 'Prettier --write' to fix.
⏰ 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). (1)
- GitHub Check: check_release
🔇 Additional comments (2)
jest.config.js (1)
7-7: LGTM! ES module configuration updated correctly.Additions of
@deepnoteandyamlto the transform whitelist align with the new test dependencies.Also applies to: 15-15
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
7-248: Test coverage and type safety look solid.Comprehensive edge-case coverage (truncation, immutability, metadata handling). No explicit
anytypes present—past review concerns appear resolved.
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/__tests__/NotebookPicker.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-118)
🪛 ast-grep (0.39.6)
src/__tests__/NotebookPicker.spec.ts
[warning] 152-152: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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
- GitHub Check: check_release
🔇 Additional comments (6)
src/__tests__/NotebookPicker.spec.ts (6)
4-9: Type-only import pattern looks good.Using
import typefor NotebookPanel aligns with TypeScript best practices and the stricter ESLint rules.
10-30: Well-structured mock types.These interfaces eliminate implicit
anytypes and align with the stricter ESLint rules. If these mocks are reused in other test files, consider extracting them to a shared test utilities module.
33-34: Type annotations improve safety.Explicit mock types replace implicit
anyand prevent type errors.
63-67: Type casting pattern is acceptable for tests.The double cast through
unknownis a standard approach when mocks don't fully implement the real interface. The comment clarifies the override purpose.
82-82: Optional chaining prevents potential errors.Safer array access with
?.operator.
166-184: Comprehensive error path coverage.Test properly verifies error logging and prevents invalid metadata from triggering
fromJSON. Spy cleanup is handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
119-135: Add test for constructor error handler.The error handler in the
NotebookPickerconstructor (.catch()block triggered whenpanel.context.readyrejects) is not tested. The PR summary claims this test was added, but it's absent from the test file. All five existing tests cover successful paths only.Add a test that:
- Mocks
panel.context.readyto reject- Verifies the component initializes without crashing
- Optionally verifies error logging occurs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/NotebookPicker.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-118)
🔇 Additional comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
10-30: Mock types are well-structured.The typed mocks properly support the new ESLint rules by avoiding
anytypes and providing explicit type safety for test fixtures.
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/handler.ts(1 hunks)
🔇 Additional comments (1)
src/handler.ts (1)
33-48: Type safety improvements look solid.Changes eliminate
any, add defensive guards, and use proper type narrowing for message extraction. Well done.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/__tests__/NotebookPicker.spec.ts (2)
81-83: Remove redundant optional chaining after existence/length checks.You’ve already asserted the select and options length.
- expect(select?.options.length).toBe(2); - expect(select?.options[0]?.value).toBe('Notebook 1'); - expect(select?.options[1]?.value).toBe('Notebook 2'); + expect(select.options.length).toBe(2); + expect(select.options[0].value).toBe('Notebook 1'); + expect(select.options[1].value).toBe('Notebook 2');
198-204: Make the null-model behavior explicit.Add a state assertion so the test doesn’t only “not throw”.
await framePromise(); expect(mockNotebookModel.fromJSON).not.toHaveBeenCalled(); + // Explicitly verify UI remained consistent despite null model + expect(select.value).toBe('Notebook 2');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/NotebookPicker.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-118)
🔇 Additional comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
10-14: Overall: solid coverage and alignment with stricter lint rules.Tests are clear and robust with minor nits above.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/handler.ts (1)
28-30: Good fix: normalize to TypeError for NetworkError.Matches the constructor’s expected type. Nice cleanup.
src/__tests__/NotebookPicker.spec.ts (5)
16-37: Generalize the panel factory and shape‑check the mock.Accept overrides and use satisfies to keep types honest; reduces double casts and ad‑hoc mocks.
- const createMockPanel = (metadata: PartialJSONObject): NotebookPanel => { + const createMockPanel = ( + metadata: PartialJSONObject, + opts: { ready?: Promise<void>; model?: Partial<INotebookModel> | null } = {} + ): NotebookPanel => { deepnoteMetadata = metadata; mockNotebookModel = { fromJSON: jest.fn(), dirty: false, getMetadata: jest.fn((key: string) => { if (key === 'deepnote') { return deepnoteMetadata; } return undefined; }) }; - return { - context: { - ready: Promise.resolve(), - model: mockNotebookModel as INotebookModel - }, - model: mockNotebookModel as INotebookModel - } as unknown as NotebookPanel; + const ready = opts.ready ?? Promise.resolve(); + const modelForPanel = + (opts.model ?? (mockNotebookModel as INotebookModel)) as INotebookModel | null; + const panelLike = { + context: { + ready, + model: mockNotebookModel as INotebookModel + }, + model: modelForPanel + } satisfies Pick<NotebookPanel, 'context' | 'model'>; + return panelLike as unknown as NotebookPanel; };
30-37: Avoidas unknown as NotebookPanel; keep a single, shape‑checked assertion.Prefer returning a
Pick<NotebookPanel, 'context' | 'model'>that satisfies the shape, then assert once at return.- } as unknown as NotebookPanel; + } as NotebookPanel;
76-81: Cast the select element and drop redundant optional chaining.You already assert non‑null; cast to HTMLSelectElement and access directly.
- const select = widget.node.querySelector('select'); + const select = widget.node.querySelector('select') as HTMLSelectElement; expect(select).not.toBeNull(); - expect(select?.options.length).toBe(2); - expect(select?.options[0]?.value).toBe('Notebook 1'); - expect(select?.options[1]?.value).toBe('Notebook 2'); + expect(select.options.length).toBe(2); + expect(select.options[0].value).toBe('Notebook 1'); + expect(select.options[1].value).toBe('Notebook 2');Apply similarly in the other blocks.
Also applies to: 91-95, 105-109
182-193: Reuse the factory for the null‑model case.Keeps tests DRY and avoids bespoke unsafe casts.
- const nullModelPanel = { - context: { - ready: Promise.resolve(), - model: { - getMetadata: jest.fn(() => deepnoteMetadata) - } - }, - model: null - } as unknown as NotebookPanel; + const nullModelPanel = createMockPanel(deepnoteMetadata, { model: null });
223-231: Reuse the factory to simulate initialization failure.Isolates state and simplifies setup.
- const failingPanel = { - context: { - ready: Promise.reject(new Error('Initialization failed')), - model: { - getMetadata: jest.fn(() => ({})) - } - }, - model: mockNotebookModel as INotebookModel - } as unknown as NotebookPanel; + const failingPanel = createMockPanel({}, { + ready: Promise.reject(new Error('Initialization failed')) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/NotebookPicker.spec.ts(1 hunks)src/handler.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-118)
dinohamzic
requested review from
Artmann,
dinohamzic and
saltenasl
as code owners
saltenasl
left a comment
saltenasl
left a comment
There was a problem hiding this comment.
one test logic improvement suggestion
| simulate(select, 'change', { target: { value: 'nonexistent' } }); | ||
| await framePromise(); | ||
| expect(model.fromJSON).not.toHaveBeenCalled(); | ||
| expect(mockNotebookModel.dirty).toBe(false); |
There was a problem hiding this comment.
the default is already dirty = false (line 21)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
81-83: Remove redundant optional chaining after length assertion.Line 81 confirms
options.lengthis 2, sooptions[0]andoptions[1]cannot be undefined. The?.on lines 82-83 is defensive but unnecessary.Apply this diff:
expect(select?.options.length).toBe(2); - expect(select?.options[0]?.value).toBe('Notebook 1'); - expect(select?.options[1]?.value).toBe('Notebook 2'); + expect(select?.options[0].value).toBe('Notebook 1'); + expect(select?.options[1].value).toBe('Notebook 2');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/NotebookPicker.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-118)
🔇 Additional comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
1-292: Test suite is comprehensive and correct.The tests thoroughly cover NotebookPicker behavior: rendering, selection, initialization errors, and metadata validation. The switch from
setTimeout(0)toPromise.resolve()(line 48) properly addresses past feedback. The refactor suggestions above (factory pattern for special cases, optional chaining cleanup) would improve maintainability but aren't blockers.
* Add ESLint rules for type safety and fix violations - Add @typescript-eslint/no-floating-promises: error - Add @typescript-eslint/no-non-null-assertion: error - Add @typescript-eslint/prefer-nullish-coalescing: error - Change @typescript-eslint/no-explicit-any from off to error Fixes: - Fix floating promise in NotebookPicker constructor by adding .catch() - Fix non-null assertion in NotebookPicker.onAfterAttach with null check - Fix prefer-nullish-coalescing in handler.ts by using ?? instead of || - Add inline eslint-disable comments for legitimate any usage in handler.ts * Add test for error handling in NotebookPicker constructor Improves test coverage for the .catch() error handler added to handle promise rejections in the constructor. * Add comprehensive test coverage for NotebookPicker - Add test for null model in handleChange - Add test for invalid metadata in handleChange - Add test for onAfterAttach without parent - Import Message type for test Coverage improved from 84.09% to 97.72% for NotebookPicker.tsx * Merge test coverage from PR #45 Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage * fix: remove mocks, test real implementation * chore: format * chore: remove out of scope tests * chore: remove out of scope test * refactor: fix test and format * chore: remove out of scope test * fix: address violations * refactor: improve test * refactor: TypeError * fix: drop unnecessary property * refactor: simplify promise * chore: improve "dirty" test --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: dinohamzic <dino@subtlebits.com>
* Add comprehensive test coverage for core transformation functions - Add tests for convert-deepnote-block-type-to-jupyter.ts (100% coverage) - Add tests for convert-deepnote-block-to-jupyter-cell.ts (100% coverage) - Add tests for transform-deepnote-yaml-to-notebook-content.ts (100% coverage) This increases overall test coverage from 26.57% to 55.94%, providing comprehensive testing for the core Deepnote-to-Jupyter transformation logic. * Address CodeRabbit review comments - Add assertions to verify createPythonCode is called correctly in code cell tests - Add assertions to verify createMarkdown is called correctly in markdown cell tests - Add assertions to verify convertDeepnoteBlockToJupyterCell is called for all notebooks in transform tests - Verify mock call counts and arguments to strengthen test contracts * fix: Add ESLint rules for type safety and fix violations (#46) * Add ESLint rules for type safety and fix violations - Add @typescript-eslint/no-floating-promises: error - Add @typescript-eslint/no-non-null-assertion: error - Add @typescript-eslint/prefer-nullish-coalescing: error - Change @typescript-eslint/no-explicit-any from off to error Fixes: - Fix floating promise in NotebookPicker constructor by adding .catch() - Fix non-null assertion in NotebookPicker.onAfterAttach with null check - Fix prefer-nullish-coalescing in handler.ts by using ?? instead of || - Add inline eslint-disable comments for legitimate any usage in handler.ts * Add test for error handling in NotebookPicker constructor Improves test coverage for the .catch() error handler added to handle promise rejections in the constructor. * Add comprehensive test coverage for NotebookPicker - Add test for null model in handleChange - Add test for invalid metadata in handleChange - Add test for onAfterAttach without parent - Import Message type for test Coverage improved from 84.09% to 97.72% for NotebookPicker.tsx * Merge test coverage from PR #45 Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage * fix: remove mocks, test real implementation * chore: format * chore: remove out of scope tests * chore: remove out of scope test * refactor: fix test and format * chore: remove out of scope test * fix: address violations * refactor: improve test * refactor: TypeError * fix: drop unnecessary property * refactor: simplify promise * chore: improve "dirty" test --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: dinohamzic <dino@subtlebits.com> * chore: improve test, avoid unnecessary mocks * chore: remove copyright comment * chore: improve test by avoiding any mocking * chore: new line --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: dinohamzic <dino@subtlebits.com>
Add ESLint rules for type safety and fix violations
Summary
Added four new TypeScript ESLint rules to catch common type safety issues and fixed all existing violations in the codebase:
New ESLint rules:
@typescript-eslint/no-floating-promises: "error"- Prevents unhandled promises that could silently fail@typescript-eslint/no-non-null-assertion: "error"- Prevents unsafe use of the!operator@typescript-eslint/prefer-nullish-coalescing: "error"- Enforces??over||for null/undefined checks@typescript-eslint/no-explicit-any: "error"(changed from "off") - Catches unsafeanyusageViolations fixed:
.catch()handler for promise rejection (previously usedvoidwhich suppressed the floating promise error but didn't handle failures)this.parent!non-null assertion with proper null checkdata.message || datatodata.message ?? data(more semantically correct for null/undefined checks)anyusages (error type in catch block and JSON parsing result)Test coverage: Added test case for the new error handler in NotebookPicker to improve coverage from 81.81% to 84.09%.
Review & Testing Checklist for Human
.catch()logs errors to console instead of letting them fail silently.this.parentinonAfterAttachdoesn't break widget resizing behavior in the notebook toolbar.||to??in error message fallback is semantically different -??only checks null/undefined while||checks all falsy values. Verify this is correct for error message handling (it should be, but worth confirming).Test Plan
.deepnotefile in JupyterLab with the extension enabledNotes
anyis reasonable (error catching and JSON parsing)Devin session: https://app.devin.ai/sessions/caa082fdab844fd988985a46cb61b547
Requested by: James Hobbs (@jamesbhobbs)
Summary by CodeRabbit
Bug Fixes
Tests
Chores