feat(@deepnote/database-integrations): extract reusable integration loading#394
Merged
Conversation
…oading Move the integration loading pipeline (YAML parsing, .env secret resolution, cloud-pull/fetch, and comment-preserving YAML/.env write-back) out of @deepnote/cli into @deepnote/database-integrations so the CLI and the VS Code extension can load and store integrations the same way. - Root export is browser-safe; Node fs/process.env wrappers live behind a new @deepnote/database-integrations/node subpath. File-touching functions are split into a content-accepting core (parseIntegrations, parseDotEnv, parseIntegrationsDocument, ...) plus thin path wrappers (parseIntegrationsFile, readDotEnv/updateDotEnv, ...). - @deepnote/cli integration modules become thin re-export shims; the `run` command and CLI behavior are unchanged. Integration tests moved into the package. - Reuse @deepnote/blocks' strict parseYaml/decodeUtf8NoBom for parity. - Reimplement .env parsing (no dotenv dependency) to keep the root browser-safe, since dotenv eagerly requires node:fs. - MergeResult now reports skipped (invalid/unsupported) integrations so callers can surface them. Bumps @deepnote/database-integrations to 1.5.0. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
- Coverage 83.95% 83.94% -0.02%
==========================================
Files 146 151 +5
Lines 8029 8047 +18
Branches 2168 2236 +68
==========================================
+ Hits 6741 6755 +14
- Misses 1287 1291 +4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…tabase-integrations directly
Drop the backward-compat re-export shims left behind by the integration
loading extraction and have every CLI consumer import from
@deepnote/database-integrations (browser-safe root) or its /node subpath
directly.
- Delete the 9 re-export shims (constants, utils/{api,dotenv,env-var-refs},
integrations/{fetch-integrations,inject-integration-env-vars,
integrations-file-schemas,merge-integrations,parse-integrations}) and the
integrations barrel
- Remove the re-export blocks from commands/integrations.ts and validate.ts
- Repoint all consumers, dynamic import() calls, and the generate-json-schemas
script
- Retarget vitest mocks to the package root and /node specifiers
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…for environment variable resolution - Updated the environment variable resolution logic to prioritize real environment variables over those defined in the .env file. - Enhanced documentation comments for clarity on the environment variable resolution process. - Added tests to verify that environment variables from the .env file are correctly resolved and that process.env can override these values.
Move the Node fs/process.env wrappers out of @deepnote/database-integrations/node back into the CLI, leaving the package as a single browser-safe entry point. The VS Code extension will provide its own filesystem wrappers around the same browser-safe core. - readDotEnv/updateDotEnv -> cli src/utils/dotenv.ts, using the dotenv library directly (drops the custom parseDotEnv reimplementation) - resolveEnvVarRefs -> cli src/utils/env-var-refs.ts - parseIntegrationsFile/getDefaultIntegrationsFilePath -> cli src/integrations/parse-integrations.ts - readIntegrationsDocument/writeIntegrationsFile -> cli src/integrations/integrations-document.ts - injectIntegrationEnvVars -> cli src/integrations/inject-integration-env-vars.ts Remove the ./node export, the dotenv dependency, and the second tsdown entry from @deepnote/database-integrations. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Restore the non-extracted CLI code to its original main location and content, so the diff reflects only code extracted into @deepnote/database-integrations (plus CLI imports updated to consume it), not incidental code movement. - utils/dotenv.ts: restored verbatim from main (not extracted) - integrations/inject-integration-env-vars.ts: back to main content - utils/env-var-refs.ts + .test.ts: keep only resolveEnvVarRefs in place, importing the extracted resolveEnvVarRefsFromMap from the library - readIntegrationsDocument/writeIntegrationsFile stay in commands/integrations.ts; drop the interim integrations-document.ts module - run.test.ts / merge-integrations.test.ts: re-point only the extracted symbols to the library, keep CLI-local mocks/imports as on main Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- Extracted the DEEPNOTE_TOKEN_ENV constant into a new constants.ts file for better organization. - Updated all relevant imports across CLI and command files to reference the new location of DEEPNOTE_TOKEN_ENV. - Cleaned up imports in integrations and run command files to remove direct imports from @deepnote/database-integrations. This change enhances code maintainability and clarity by centralizing the environment variable definition.
Keep API fetch orchestration in the CLI where logging and machine-output handling live; the library continues to export fetchIntegrations and convertApiIntegrations as the shared primitives. Co-authored-by: Cursor <[email protected]>
Separate skipped integrations from the base merge result so mergeProcessedIntegrations no longer returns a dummy empty array, and rename result types for clarity. Co-authored-by: Cursor <[email protected]>
…ata helper The parameter only existed for a CLI error log removed during the integration loading extraction. Co-authored-by: Cursor <[email protected]>
dinohamzic
approved these changes
Jun 12, 2026
Contributor
Author
|
@coderabbitai review |
Contributor
Author
|
@coderabbitai full review |
Contributor
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
@coderabbitai approve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
(ᵔ ᵕ ᵔ)⌐■-■ ✅ Action performedComments resolved and changes approved. |
Contributor
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)
packages/cli/src/commands/integrations.ts (1)
152-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserved-count math should exclude skipped API integrations.
preservedCountcurrently subtracts all fetched integrations, including skipped invalid/unsupported ones, so the summary can undercount preserved local-only entries.Suggested fix
- const preservedCount = finalCount - fetchedIntegrations.length + const mergedFetchedCount = fetchedIntegrations.length - skipped.length + const preservedCount = finalCount - mergedFetchedCount🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/integrations.ts` around lines 152 - 154, The preservedCount calculation in the integrations command is incorrectly including skipped invalid or unsupported API integrations in the subtraction. Fix the calculation by subtracting only the actually-processed/valid integrations from finalCount instead of subtracting the entire fetchedIntegrations.length. You should track how many integrations were successfully processed (excluding those that were skipped due to being invalid or unsupported) and use that count in the preservedCount calculation to accurately reflect how many local-only integrations were preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/integrations/parse-integrations.test.ts`:
- Around line 468-478: Import dedent from 'ts-dedent' at the top of the file if
not already present. Then, wrap the multiline YAML template string literals
passed to writeFile at lines 468 and 498 with the dedent function call, changing
from backtick strings directly to dedent`` template strings. This ensures
consistent formatting and removes unnecessary indentation from the YAML content
blocks, following the established pattern used elsewhere in the codebase.
In `@packages/cli/src/integrations/parse-integrations.ts`:
- Around line 52-54: Wrap the readDotEnv function call (line 53) in a try-catch
block to handle permission denied, parse failures, and other file I/O errors. In
the catch block, return a structured issues result that aligns with the existing
error handling pattern used for integrations file failures, rather than allowing
these errors to propagate uncaught. This ensures consistent error handling
across both the envFilePath and integrations file paths.
---
Outside diff comments:
In `@packages/cli/src/commands/integrations.ts`:
- Around line 152-154: The preservedCount calculation in the integrations
command is incorrectly including skipped invalid or unsupported API integrations
in the subtraction. Fix the calculation by subtracting only the
actually-processed/valid integrations from finalCount instead of subtracting the
entire fetchedIntegrations.length. You should track how many integrations were
successfully processed (excluding those that were skipped due to being invalid
or unsupported) and use that count in the preservedCount calculation to
accurately reflect how many local-only integrations were preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 644f4708-aa38-4f45-9073-bad5c8ecf584
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
packages/cli/scripts/generate-json-schemas.tspackages/cli/src/cli.tspackages/cli/src/commands/integrations.test.tspackages/cli/src/commands/integrations.tspackages/cli/src/commands/integrations/add-integration.tspackages/cli/src/commands/integrations/edit-integration.tspackages/cli/src/commands/lint.test.tspackages/cli/src/commands/run.test.tspackages/cli/src/commands/run.tspackages/cli/src/commands/validate.tspackages/cli/src/constants.tspackages/cli/src/integrations/collect-integrations.tspackages/cli/src/integrations/fetch-and-merge-integrations.tspackages/cli/src/integrations/index.tspackages/cli/src/integrations/parse-integrations.test.tspackages/cli/src/integrations/parse-integrations.tspackages/cli/src/utils/analysis.tspackages/cli/src/utils/env-var-refs.test.tspackages/cli/src/utils/env-var-refs.tspackages/database-integrations/package.jsonpackages/database-integrations/src/index.tspackages/database-integrations/src/loading/api-error.tspackages/database-integrations/src/loading/constants.tspackages/database-integrations/src/loading/env-var-refs.test.tspackages/database-integrations/src/loading/env-var-refs.tspackages/database-integrations/src/loading/fetch-integrations.test.tspackages/database-integrations/src/loading/fetch-integrations.tspackages/database-integrations/src/loading/index.tspackages/database-integrations/src/loading/integrations-document.tspackages/database-integrations/src/loading/integrations-file-schema.tspackages/database-integrations/src/loading/merge-integrations.test.tspackages/database-integrations/src/loading/merge-integrations.tspackages/database-integrations/src/loading/merge-into-yaml.tspackages/database-integrations/src/loading/parse-integrations.tspackages/database-integrations/src/loading/validation-issue.ts
💤 Files with no reviewable changes (2)
- packages/cli/src/constants.ts
- packages/cli/src/integrations/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Extracts the browser-safe integration loading logic out of
@deepnote/cliinto@deepnote/database-integrations, so the CLI and the VS Code extension can parse, fetch, and write integrations the same way. The extension already depends on@deepnote/database-integrations(it consumesgetEnvironmentVariablesForIntegrations), so this is the natural home for the shared logic.The split is along the Node boundary: everything pure (works in the browser and Node) moves into the package, while the thin Node
fs/process.envwrappers and the CLI's fetch-and-merge orchestration stay in the CLI. The package root keeps nonode:imports, so the extension can import it directly (feeding it strings it reads viaworkspace.fs) without any Node-only entry point.What moved into
@deepnote/database-integrationsNew
src/loading/directory, re-exported from the package root (export * from './loading'):parseIntegrations({ yaml, env })— parse the integrations YAML and resolveenv:references against a supplied mapresolveEnvVarRefsFromMap+ theenv:ref helpers (parseEnvVarRef,createEnvVarRef,isEnvVarRef,extractEnvVarName,generateEnvVarName,EnvVarResolutionError)fetchIntegrations+ the API response schemas (apiIntegrationSchema,apiResponseSchema)convertApiIntegrations,mergeApiIntegrationsIntoDocument, the comment-preserving YAML document builders, and the merge-result typesmergeApiIntegrationsIntoYaml— string-in / string-out write-back convenienceparseIntegrationsDocument/serializeIntegrationsDocumentintegrationsFileSchema,ValidationIssue,ApiError(moved from the CLI'sutils/api.ts), and constants (BUILTIN_INTEGRATIONS,DEFAULT_ENV_FILE,DEFAULT_INTEGRATIONS_FILE,DEFAULT_API_URL)The package gains
@deepnote/blocks(workspace) andyamlas dependencies, and reuses@deepnote/blocks' strictparseYamlfor exact behavior parity (browser-safe, no dependency cycle).What stays in
@deepnote/cliThe Node-specific glue, now thin wrappers over the extracted functions:
parseIntegrationsFile/getDefaultIntegrationsFilePath— read the file from disk (decoding via@deepnote/blocks'decodeUtf8NoBom), overlay.env+process.env, then delegate toparseIntegrationsresolveEnvVarRefs(obj)— wrapsresolveEnvVarRefsFromMap(obj, process.env)fetchAndMergeApiIntegrations— the CLI's orchestration overfetchIntegrations+convertApiIntegrations.envread/write (utils/dotenv.ts), still backed by thedotenvpackage — keepingdotenv's eagernode:fsrequire out of the package rootThe now-removed
integrations/index.tsbarrel's consumers (run,validate,integrations,add/edit-integration,collect-integrations,analysis, …) import from@deepnote/database-integrationsdirectly. Theruncommand and overall CLI behavior are unchanged.Tests
Pure-logic suites moved into the package alongside the code (
env-var-refs,fetch-integrations,merge-integrations). The CLI keeps the tests for its remaining Node wrappers (parse-integrationsfile reader,dotenv, theenv-var-refswrapper).Notable decisions
.envparsing stays in the CLI viadotenv, so the package root stays browser-safe.IntegrationsMergeResult(secrets+stats) →IntegrationsDocumentMergeResult(addsskipped: SkippedApiIntegration[]) →IntegrationsYamlDocumentMergeResult— so callers can surface invalid/unsupported integrations that were skipped./nodesubpath and no version bump (stays1.4.3): because the path/process.envwrappers stay in the CLI, the package root is browser-safe by construction and a separate Node entry point proved unnecessary.Verification
pnpm -r run build— all 7 packages build ✓@deepnote/database-integrations+@deepnote/cli): 1200 passed / 79 files ✓pnpm run typecheck(roottsc -p tsconfig.json+ per-packagetsc --noEmit) ✓biome check .— no errors ✓dist/index.js+dist/index.cjs) has nonode:/fs/path/os/processimports ✓Not included
VS Code extension wiring (
/workspace/vscode-deepnote) is intentionally out of scope; it can consume the new@deepnote/database-integrationsexports as a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
integrations pullto provide detailed reporting for skipped, invalid, and unsupported integrations, including names, types, IDs, and specific issues.envFilePathfor resolvingenv:references from a dotenv file, with process environment values taking precedence.