feat(deepnote): load integrations from .deepnote.env.yaml and .env - #440
Open
tkislan wants to merge 147 commits into
Open
feat(deepnote): load integrations from .deepnote.env.yaml and .env#440tkislan wants to merge 147 commits into
tkislan wants to merge 147 commits into
Conversation
…d) + add project-id resolver
Chunk 1 of single-notebook migration (§4 partial, §5). No behaviour change.
- Manager caches originals in a nested Map<projectId, Map<notebookId, project>>
so sibling files sharing a project.id no longer clobber each other.
- New API: getOriginalProject(projectId, notebookId) exact/no-fallback,
getAnyProjectEntry(projectId), storeOriginalProject/updateOriginalProject
(3-arg), updateProjectIntegrations iterates all entries.
- Update IDeepnoteNotebookManager and IPlatformDeepnoteNotebookManager; repoint
all project-level read-only callers to getAnyProjectEntry.
- Add canonical readDeepnoteProjectFile and resolveProjectIdFor{File,Notebook}.
- Selection state and init-run tracking intentionally kept (removed in later chunks).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…rop selection machinery Chunk 2 of single-notebook migration (§1 + Cleanup). - deserializeNotebook renders the first non-init notebook (findDefaultNotebook), falling back to the only/init notebook; never composes init. - serializeNotebook resolves the target from document metadata alone (projectId + notebookId required) and looks it up with the exact getOriginalProject, throwing clear errors instead of falling back to a wrong sibling. - detectContentChanges collapses to a single-notebook comparison. - Remove the ?notebook=<id> selection machinery: findCurrentNotebookId, the manager's selection state + interface methods, the explorer's query-param opens and selectNotebookForProject calls, and the tree item's custom resourceUri. - Explorer no longer depends on IDeepnoteNotebookManager. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…k siblings Chunk 3 of single-notebook migration (§0, §2, §3). - Add allocateSiblingUri: the single filesystem-aware, collision-safe sibling filename allocator (bumps -2/-3 before .deepnote, honors an in-batch reserved set, bounded retries). - Add a notebook file factory (buildSingleNotebookFile / buildSiblingNotebookFileUri) for creating sibling single-notebook files (wired into the explorer in a later chunk). - Add DeepnoteMultiNotebookSplitter: on opening a multi-notebook .deepnote file, offer to split it into one new single-notebook file per notebook. The action flushes the editor if dirty, writes all children, migrates the environment selection, then closes the tab and deletes the original to trash. A child-write failure leaves the original intact (write-before-delete). - Wire the splitter into activation with an optional (desktop-only) environment mapper; add a refresh() passthrough on the explorer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
Chunk 4 of single-notebook migration (§6). - Add DeepnoteProjectMetadataPropagator (desktop): given a project id and a project-level mutator, enumerate every sibling .deepnote file on disk (open or closed), apply the change, and write it back. Skips no-op writes, refreshes the manager cache for open siblings, and collects per-file failures instead of aborting. Fires an onFileWritten hook so the file watcher treats each write as a self-write (no reload/save storm). - Route integration updates and project rename through the propagator so closed siblings stay consistent; web falls back to the cache-only / single-file paths. - Expose getOriginalProject/updateOriginalProject on the platform manager interface. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…us bar Chunk 5 of single-notebook migration (§7). - Tree is grouped: ProjectGroup (by project id) -> ProjectFile -> Notebook. A single-notebook file is a leaf labelled with its notebook; legacy multi-notebook files stay collapsible. The init notebook is excluded from counts everywhere. - Refresh is grouping-safe: refreshNotebook evicts every sibling cache entry for a project id and all refreshes fire a full-tree change (no per-item fires). - Commands are project-scoped vs notebook-scoped; new/duplicate/add-notebook create sibling files via the factory (never appended), delete removes the file for a single-notebook file, and notebook names are unique within a project group. - Add a status bar item showing the active Deepnote notebook with a "Copy Active Deepnote Notebook Details" command. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
Chunk 6 of single-notebook migration (§8). - Key the server starter maps, the config handle, and the kernel auto-selector by notebook.uri.toString() - the same identity the kernel and controller use - so a notebook's server is 1:1 with its kernel. Sibling notebooks of one project no longer share a server; the working directory and SQL env are taken from each notebook's own file. - Fix environment deletion: stop every server using the environment (including closed notebooks whose server is still running) before removing the mappings, driven from the notebook->environment mapper. Drop the dead environmentServers map that was never populated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…le reader Chunk 7a of single-notebook migration (§9). - Write snapshots with notebook-scoped filenames via @deepnote/convert (generateSnapshotFilename / parseSnapshotFilename), replacing the local slug and filename regex. - readSnapshot resolves snapshots path-free (it runs at deserialize, which has no URI): glob by project id, rank the notebook-scoped match first and keep legacy project-scoped snapshots as a fallback, and skip an empty-output "latest" (save race) or a corrupt file while walking candidates. Legacy snapshots are read, never migrated or deleted. - Defer the execution snapshot save until outputs settle (quiet window with a max wait) and cancel it on re-execute / close. - Use convert's computeSnapshotHash on the save path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
Chunk 7b of single-notebook migration (§10). - The init runner now subscribes to kernel start and restart events and runs the init notebook found in its own sibling .deepnote file (matched by project id + initNotebookId via isValidSiblingInitCandidate), instead of looking it up in the main file's notebooks. - Track "init has run" per kernel in a WeakSet<IKernel>: a fresh kernel runs init once, and an in-place restart (which fires onDidRestartKernel) re-runs it so the kernel is re-initialized before the next user cell. A missing sibling is logged and skipped without permanently marking the project. - Remove the manager's persistent init-run tracking and the selector's init staging; the runner owns init triggering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
- void the fire-and-forget onExecutionComplete call (no-floating-promises), matching the existing void performSnapshotSave pattern. - Use American "behavior" in a comment. - Add test-only technical words (basenames, initmain, Résumé, unparseable) to cspell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…of duplicating it The mocha ESM loader wholesale-mocked @deepnote/convert and reimplemented its pure helpers (resolveSnapshotNotebookId, splitByNotebooks, isValidSiblingInitCandidate, snapshot filename generate/parse, hashing, etc.). That duplicated upstream logic with no drift detection: if convert changed, the mock silently kept the old behavior and tests stayed green against a fiction. - Remove the @deepnote/convert interception from build/mocha-esm-loader.js so unit tests exercise the real package's pure functions (and now track its actual API). - Mock only the one genuinely side-effecting export, convertIpynbFilesToDeepnoteFile (real node:fs I/O), via esmock in the explorer import suites where it is used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…r paths From the Codex review of the PR (F1/F3/F4/F5), all independently verified: - F1 (P1): snapshot save fetched the cached project with getAnyProjectEntry(projectId), which can return the wrong sibling when multiple single-notebook siblings of one project are open, silently skipping the snapshot write. Use the exact getOriginalProject(projectId, notebookId) lookup instead. - F3: collectNotebookNamesForProject globbed **/*.deepnote without skipping snapshot sidecars, so stale snapshot notebook names polluted the name-uniqueness set. Filter snapshot files (matching the tree provider and propagator). - F4: detectContentChanges compared notebooks[0]; for a legacy [init, main] file the edited notebook is not at index 0, so edits were missed and modifiedAt preserved. Match the notebook by id. - F5: the deferred-save timer fired performSnapshotSave as a floating promise; wrap the save body in try/catch/finally so a build/write failure is logged (not an unhandled rejection) and execution state is always cleared. Adds regression tests for F1 (exact lookup), F3 (snapshot exclusion), and F4 (match by id). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…el init runs Addresses round-2 code-review findings G2 and G3 (both verified P2). - G2: deepnoteFileChangeWatcher's snapshot block-id recovery used the project-only getAnyProjectEntry(projectId), which can return a different open sibling's cached project (siblings share project.id), leaving originalBlocks undefined and silently skipping recovered outputs. Use the exact getOriginalProject(projectId, notebookId) — the same fix already applied to snapshotService (F1), here in the watcher path that was missed. - G3: moving init execution to the event-driven runner dropped the notebook-close cancellation that the kernel auto-selector used to provide, so closing a notebook mid-init left the remaining init blocks executing against a closed notebook. Tie the init run to a CancellationTokenSource cancelled on notebook close and dispose it in a finally. Adds regression tests for both (each fails on the pre-fix code). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
…ort/delete Removes two pieces of functionality; also folds in the branch's in-progress updates this work was layered on top of (they could not be isolated, as the removals are interleaved with and built on top of that WIP). Removed - project-metadata propagator: - Delete DeepnoteProjectMetadataPropagator and its types, drop the DI binding, and unwire it everywhere (activation, file-change watcher self-write hook, integration webview, explorer rename). Project-level fields are no longer fanned out across sibling files: each notebook owns its own integrations, and project-name drift is accepted for now. Drop the now-dead updateOriginalProject manager method and two stale comments. Removed - project-level explorer commands: - Delete the exportProject and deleteProject commands (constants, command-arg type, registrations, package.json command defs + sidebar menus, nls titles, and their unit tests). Per-notebook export remains via the existing exportNotebook command (first non-init notebook of the file). Also includes the branch's pending updates the above was built on: dependency bumps (incl. @deepnote/convert 4.0), the getOriginalProject -> getProjectForNotebook manager rename and getAnyProjectEntry removal, and assorted snapshot/serializer/kernel adjustments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Brings in #432 (Cloud SQL integration support). Resolved the package.json and package-lock.json conflicts by keeping this branch's newer @deepnote/* versions (blocks 4.6.0, convert 4.0.0, runtime-core 0.4.0); @deepnote/database-integrations is 1.5.0 on both sides, and the Cloud SQL source from #432 merged cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
@deepnote/blocks@4.6.0+ renders `text-cell-bullet` blocks with `indent_level >= 1` using leading spaces (two per level) before the bullet marker. stripMarkdown's bullet regex only matches at column 0, so the leading indentation must be trimmed first for the plain-text cell value to round-trip correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
…es re-exports snapshotFiles.ts re-exported six snapshot-filename helpers from @deepnote/convert. Remove the re-export block and import the helpers directly from @deepnote/convert at each use site (snapshotService.ts and the snapshotFiles unit test). snapshotFiles.ts now keeps only its local helpers (SNAPSHOT_FILE_SUFFIX, isSnapshotFile, extractProjectIdFromSnapshotUri) plus the single internal use of parseSnapshotFilename. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Refactor the buildSnapshotPath method to accept an object as an argument, improving readability and maintainability. Update all relevant calls to this method throughout the snapshotService and its unit tests to match the new signature. This change enhances the clarity of parameter usage and reduces the risk of errors when passing arguments.
Trim the single-notebook test suites by removing duplicate and tautological tests and collapsing/merging several others, shrinking the PR's test additions by ~640 lines with no loss of real coverage. Cuts target only tests this branch added: - exact-(projectId, notebookId)-lookup restatements duplicated across the watcher, serializer, snapshot, and manager suites - wrapper tests already covered by the delegate's own tests (addNotebookToProject, sibling-file allocation, project-id resolution) - tautologies over trivial template/getter functions (serverUtils) - framework-registration smoke tests (status bar) Merges keep the one meaningful assertion and drop the duplicate scaffolding (e.g. legacy-delete no-op folded into the existing delete test; two init builders parametrized into one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
When splitting a legacy multi-notebook .deepnote file into single-notebook
siblings, rename the original to `<name>.deepnote.legacy` instead of moving it
to the OS trash. The `.legacy` suffix takes it out of the extension's view (it
no longer matches `*.deepnote`) while keeping it on disk next to the split
results, so the user can restore it by removing the suffix.
Unlike `workspace.fs.delete({ useTrash: true })`, this is deterministic and does
not depend on an OS trash backend (which can be absent on headless Linux).
Collisions bump the name to `.legacy-2`, `.legacy-3`, … and the rename still
happens only after every child is durably written (write-before-retire).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test that drives the real VS Code UI through the on-open split of a legacy multi-notebook .deepnote file: it asserts the split prompt, the one-file-per-notebook result, the retained `.legacy` backup, that each sibling opens without re-prompting, and that content plus the project integration fan out into every split file. Add a `createScreenshotter(this)` helper that captures step screenshots into a per-spec directory derived from the running test file (`test/e2e/screenshots/<spec>/`), plus the `sales-analytics.deepnote` fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
…ites Add ExTester end-to-end suites covering: - opening a plain single-notebook file (opens directly, no split prompt, the status bar shows the notebook name); - splitting a multi-notebook file that declares an init notebook (the init notebook becomes its own single-notebook sibling; each main sibling still references it via initNotebookId); - the init-notebook runner: the sibling init notebook runs hidden in a main notebook's kernel so its definitions are available, and re-runs after a kernel restart. Add the quick-notes and etl-pipeline fixtures (including the pre-split extract/init siblings), and disable the kernel-restart confirmation in the E2E settings so the restart test can drive it non-interactively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test asserting the Deepnote Explorer groups sibling
.deepnote files by project: three files sharing one project.id collapse into a
single "Marketing" group ("3 files") whose leaves are the three notebooks, while
a file from a different project appears as its own group. Reads the tree by
diffing visible leaves before/after expanding (avoids the page-object library's
flaky CustomTreeItem.getChildItems). Adds the three marketing fixtures.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
When several E2E suites run in one ExTester session (as in CI, via the `*.e2e.test.js` glob), every workspace-folder open after the first failed with "Failed to open folder after 5 attempts" in the suite's `before all` hook — only the alphabetically-first suite passed. Root cause: the simple "Open Folder" dialog (files.simpleDialog.enable) navigates one directory level *toward* the typed path per OK click and only accepts the folder once the browser is AT it. The helper clicked OK once then re-opened the dialog each attempt, which reset navigation back to the default directory — for the 2nd+ open that default is the previous, now-deleted workspace, so the dialog fell back to "/" and never converged on the target. Fix: click OK repeatedly within a single dialog until the pre-open workbench element detaches (reload = folder accepted), instead of re-opening per attempt; and set `window.openFoldersInNewWindow: "off"` so "Open Folder" reuses the current window, keeping that reload detectable. Verified with four suites (16 tests) opening four folders in one session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test for the Deepnote status-bar item: it shows the active notebook's name (with the "Copy Active Deepnote Notebook Details" tooltip), hides when a non-notebook editor is focused, and — on click — copies the notebook details to the clipboard with a confirmation toast. The clipboard is verified by pasting into a scratch text file and reading it back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test for the notebook-management commands that create and rename sibling .deepnote files from the Deepnote explorer: New Notebook, Add Notebook (project-group context menu), Duplicate Notebook, and Rename Notebook — each verified by the resulting notebook name inside the sibling files plus the confirmation toast. Delete Notebook is included as a pending test: its context-menu -> native confirmation-modal interaction is unreliable to drive under ExTester (documented inline), so it is left as a manual check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test for the Deepnote integrations UI: opening "Manage Integrations" for a notebook whose project declares an integration lists it (the "Sales BigQuery" integration on the sales-analytics-revenue fixture), while a plain notebook (quick-notes) shows no such integration. Adds the sales-analytics-revenue fixture (a single-notebook split of the Sales Analytics project carrying the BigQuery integration + its SQL cell). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test for renaming a Deepnote project from the Explorer: none of the three "Marketing" siblings is opened, then the project group is renamed to "Growth" via its context menu; the new name is asserted to fan out to every sibling .deepnote file on disk (and the old name gone), with the Explorer group relabelled and a confirmation toast. Extract the shared Deepnote tree helpers (getDeepnoteExplorerSection, readDeepnoteTreeRows, findDeepnoteGroup/Leaf, selectDeepnoteContextMenu) into test/e2e/helpers/deepnoteTree.ts for reuse across the tree-driven suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
Add an ExTester end-to-end test for the edge case where a .deepnote file's only notebook is its init notebook. Opening bootstrap-only.deepnote renders that notebook as a fallback (status bar shows "Bootstrap"), does not raise the split prompt (it is a single-notebook file), and the Explorer shows it with "0 notebooks" (the init notebook is excluded from the count). Adds the bootstrap-only fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
The loopback endpoint is now the only path by which SQL credentials reach a kernel, so a bind failure silently breaks every integration: applyIntegrationEndpointEnv skips injection when baseUrl is undefined, and the toolkit swallows set_integration_env errors with only a log. activate() previously just logged. The user-facing prompt lived in onServerError, which returns early while isListening is false -- so it never fired for an initial bind, only for a crash after listening. Route activation through startAndNotifyOnFailure() so both paths prompt, and share the dialog via promptToRecover(). Offer only Reload Window. Restarting in place would bind a NEW ephemeral port (listen(0)) while applyIntegrationEndpointEnv runs solely at server start, so already-running toolkit servers would keep pointing at the dead port -- the action would look like a fix without being one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvHKpadAcqgjxzW71TUQyP
…yaml `IntegrationsFileConfigProvider` dropped every federated-auth integration found in the environment file. That was wrong for the one combination this extension implements — BigQuery + `google-oauth` — because the file only ever carries OAuth client metadata (`project`, `clientId`, `clientSecret`), the same data SecretStorage holds. The token is a separate artifact owned by `IFederatedAuthTokenStorage`, and keeping those credentials out of the kernel environment is already `getEnvironmentVariables`' job. Dropping at parse time was redundant for that concern and hid the config from the SQL LSP and the per-cell status bar. Narrow the parse-time guard to unsupported federated methods, and make the config/derived-state boundary explicit with a second signature rather than a mode flag: `getMergedConfigs` answers what applies at runtime (read-only — the file layer cannot be written back), `getFederatedAuthCandidates` answers which ids can be authenticated, exposing no config so the integrations panel can offer the action without receiving credentials it cannot save. The two node-only federated readers now resolve against the merged configs for an explicitly supplied notebook URI instead of SecretStorage, so a file-declared integration resolves and the button's eligibility cannot disagree with the command's lookup about which notebook it means. A failed SQL cell offers Authenticate directly, since that is where the user hits it. Orphaned-token cleanup now requires witnessed removal — an id seen in a previous observation of SecretStorage and gone from the current one — rather than treating mere absence as proof. A YAML-only integration is never in SecretStorage, so without this the token was deleted at the next window load, or mid-session when any unrelated integration was saved. Runs are serialised: both the snapshot read and its replacement straddle an await, so overlapping observations could otherwise pair one run's read against another's snapshot and witness a removal that never happened. The env-var chokepoint is deliberately unchanged; its comment now explains why, since federated client metadata can now legitimately reach the merge. Refs specs/FEDERATED_AUTH_ENV_FILE_PLAN.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvHKpadAcqgjxzW71TUQyP
…ures `handleFederatedGenerateError` is pre-existing behaviour with nothing to do with reading integrations out of `.deepnote.env.yaml`, and the env-file feature works without it. Offering an Authenticate action on the failing cell is a usability change that should stand on its own merits rather than ride along in an unrelated PR. Restores the method to its original form, along with the `bigQueryNotAuthenticated` wording and the `window`/`commands`/`Commands` imports the toast introduced. The `generate(deepnoteBlock, this.cell.notebook.uri)` call site stays: the generator resolves its config per notebook from the merged file/SecretStorage view, so the URI is required by the interface, not by the reverted toast. Planned separately in specs/FEDERATED_AUTH_CELL_FAILURE_ACTION_PLAN.md (untracked), which records the design, the tests and the recovery point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvHKpadAcqgjxzW71TUQyP
…ove clarity. Removed tests for alternative field names and updated assertions for BigQuery and federated auth cases to ensure they return null as expected. Renamed 'username' to 'user' for consistency across configurations.
Tests were 61% of this branch's additions. An audit across all 21 changed test suites — backed by mutation testing against the compiled output — found the suites are well partitioned: what reads as duplication across the pipeline stages is repeated prose over genuinely separate guards, and deleting any one of them fails exactly one test. Cross-file duplication was about 4% of the added test surface. Removes what the audit could prove redundant, each with a surviving test that still fails if the behaviour regresses: - the env-file loader suite drops two tests re-proving upstream `parseIntegrations` behaviour, and its near-identical `.deepnote.env.yaml` literals collapse into one builder - the refresh handler and live refresher drop four cases inherited from the deleted `integrationKernelRestartHandler` suite, which landed at a layer that no longer decides what they describe - `userpodApiEndpoints` drops a test that killed none of 14 mutants, and its repeated start-up preamble becomes a helper - `deepnoteServerStarter` drops an env assertion already owned by `applyIntegrationEndpointEnv`'s own suite; the sibling leak test keeps its exhaustive form, so "nothing else reaches the kernel env" stays covered Also shares three fixtures that had been rebuilt per suite. The three `createMockNotebook` copies had already drifted to different argument orders, which is the drift a shared factory exists to prevent. No assertion changed and no production code was touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvHKpadAcqgjxzW71TUQyP
Replaced the manual construction of YAML strings in the pgIntegrationYaml function with a call to the dump function from the js-yaml library. This change enhances readability and maintainability by leveraging a dedicated library for YAML formatting. No changes to test assertions or production code were made.
Propagate the getMergedConfigs → getMergedIntegrationConfigs rename into the federated auth code-generator unit test mock and remaining test titles so npm run compile succeeds again.
Every path that should drop a token already does without it: the panel's delete/reset, `invalidateStaleFederatedToken` on an auth-method or fingerprint change, and the `invalid_grant` drop in the SQL codegen. That left the cleaner one job — retrying a `tokenStorage.delete` that threw — and it could not really do that either. `onDidChangeIntegrations` fires synchronously inside `integrationStorage.delete()`, so the cleaner's run started before the panel reached its own delete: a concurrent duplicate attempt at the same keychain write, not a later retry. Once that run finished the id was out of the snapshot, so no later event could witness the removal again, and the witnessed-removal rule meant a host that never saw the removal (another window, an earlier session) could not collect it at all. Nor was there anything to protect. Storage growth is not a concern at a handful of integrations, and a leftover entry is inert: a reused id with different OAuth client metadata has its token dropped on fingerprint mismatch, and an unchanged one is still the correct token. Local deletion never revoked the grant at Google, so it was housekeeping rather than a security boundary in the first place. `listIntegrationIds` had no other consumer, so it goes with it, along with the three untracked federated-auth plan documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
…WebviewProvider Removed redundant error handling for token deletion in multiple methods, ensuring that the token is deleted directly without catching errors. Updated unit tests to verify that a failed token deletion aborts configuration changes, maintaining the integrity of the integration state. This change simplifies the code and enhances error management during integration updates.
The integrations panel built its list from `project.integrations` plus SecretStorage only, so an integration configured in `.deepnote.env.yaml` displayed as "Not Configured" and offered a Configure button that wrote to SecretStorage — which the file-wins merge then silently overrode. Saving reported success and changed nothing. Such rows now read "Configured in file" with Configure, Reset and Delete hidden. Authenticate is deliberately kept: for BigQuery + `google-oauth` the token lives in SecretStorage, not the file, so it stays the one legitimate panel action. Read-only is enforced extension-side, not just in the webview. `showConfigurationForm` is reachable from the SQL cell status bar's "Configure current integration", which bypassed the hidden buttons entirely. `refuseEditIfFileConfigured` guards all four mutating entry points and re-resolves the file per action, so a stale render snapshot, a lost `updateWebview()` generation race, or a switch to another notebook cannot defeat it. A read failure fails open so a hiccup never blocks a real edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
…ocus The toolbar button's `when: notebookType == 'deepnote'` follows the visible editor, but the handler read `window.activeNotebookEditor`, which is only set once a notebook editor has been focused. A session restored with a `.deepnote` file open therefore rendered the button while the active editor was still undefined, and the command failed with "No active Deepnote notebook" until the user clicked into the notebook. Resolve in three steps instead: the URI the menu contribution passes (exact, and unambiguous with split editors), then the focused editor, then a lone visible Deepnote editor. The argument scan no longer stops at the first string, so the integration id and the toolbar context object can both be picked up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
`IntegrationDetector` built its map from `project.integrations` alone, so an integration declared only in `.deepnote.env.yaml` never appeared in the panel — even though the merge appends file-only ids additively and it works at execution time. For a federated integration that was fatal rather than cosmetic: Authenticate exists only as a row in this panel, so a file-only BigQuery + `google-oauth` integration could never obtain a token, and the carve-out that keeps Authenticate visible for file-configured rows only helped once the id was also in the roster. The detector now appends merged configs the roster omits, using the source `getMergedIntegrationConfigs` already names as an intended consumer. `config` stays `null` — the panel edits SecretStorage only and the file layer cannot be written back — so those rows render read-only, with the name and type carried through for display. `IntegrationDetectionInput` gains the notebook URI because `.deepnote.env.yaml` is located relative to the notebook and the ids alone cannot find it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
…cord them The SQL block integration picker was built from `project.integrations` alone, so an integration declared only in `.deepnote.env.yaml` could not be selected — it was reachable only by hand-editing `sql_integration_id`. The picker now unions the roster with the merged configs, which also fixes the "unknown integration" check that would otherwise have labelled a file-only integration `Unknown integration (configure)` and then listed it a second time. Picking one now also appends it to `project.integrations`, so the `.deepnote` file records what it actually uses. This is the single place the roster drifts, so reconciling here keeps it correct without writing on every save. Additive only, deliberately. Existing entries pass through verbatim rather than being re-validated: a project's integrations are shared with sibling notebooks whose blocks this cannot see, so dropping an entry — including one whose type this build does not recognise — would be data loss. Re-picking an already-declared integration writes nothing, so no `modifiedAt` churn, and a roster-only write leaves cells untouched so the file watcher does not reload. A failed write is logged rather than surfaced; the cell edit has already landed and the selection stands either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant DeepnoteServerStarter
participant DeepnoteToolkit
participant UserpodApiEndpoints
participant SqlIntegrationEnvironmentVariablesProvider
Notebook->>DeepnoteServerStarter: start notebook server
DeepnoteServerStarter->>UserpodApiEndpoints: inject endpoint URL and project token
DeepnoteToolkit->>UserpodApiEndpoints: request environment variables
UserpodApiEndpoints->>SqlIntegrationEnvironmentVariablesProvider: resolve merged configs
SqlIntegrationEnvironmentVariablesProvider-->>UserpodApiEndpoints: return SQL environment variables
UserpodApiEndpoints-->>DeepnoteToolkit: return credentials
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
103-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
getMergedIntegrationConfigshere instead of re-walking the chain.Lines 60-104 duplicate the resource → notebook → project → file-config → merge sequence that
getMergedIntegrationConfigs(Lines 172-200) already performs; the two will drift. Keep the trace logging, drop the duplicated resolution.As per coding guidelines: "Extract duplicate logic into helper methods to prevent drift following DRY principle."
♻️ Sketch
- const fileConfigs = await this.loadFileIntegrationConfigs(notebook.uri); - const allConfigs = await this.mergeIntegrationConfigs(projectIntegrations, fileConfigs); + const allConfigs = await this.getMergedIntegrationConfigs(resource, token);🤖 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 `@src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts` around lines 103 - 104, In the provider flow around loadFileIntegrationConfigs and mergeIntegrationConfigs, replace the duplicated project/notebook/resource resolution and merge sequence with a call to getMergedIntegrationConfigs. Preserve the existing trace logging, and remove only the redundant resolution logic so the shared helper remains the single implementation of this chain.Source: Coding guidelines
test/e2e/suite/integrationsEnvFileInjection.e2e.test.ts (1)
117-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the retry constants and the refreshed host to module scope.
REFRESH_MAX_ATTEMPTS/REFRESH_ATTEMPT_TIMEOUTlive inside the test body, andrefreshed-host.example.comis repeated in the.envwrite and the assertion.As per coding guidelines: "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module."
♻️ Sketch
const DOTENV_CONTENT = 'DEMO_DB_HOST=injected-host.example.com\n'; +const REFRESHED_OUTPUT = 'refreshed-host.example.com'; +const REFRESHED_DOTENV_CONTENT = `DEMO_DB_HOST=${REFRESHED_OUTPUT}\n`; +const REFRESH_MAX_ATTEMPTS = 6; +const REFRESH_ATTEMPT_TIMEOUT = 10_000;- fs.writeFileSync(path.join(tempDir, DOTENV_FILE_NAME), 'DEMO_DB_HOST=refreshed-host.example.com\n'); + fs.writeFileSync(path.join(tempDir, DOTENV_FILE_NAME), REFRESHED_DOTENV_CONTENT); @@ - const REFRESH_MAX_ATTEMPTS = 6; - const REFRESH_ATTEMPT_TIMEOUT = 10_000; let second = ''; @@ - second = await runOnceAndAwaitOutput( - NOTEBOOK_FILE_NAME, - 'refreshed-host.example.com', - REFRESH_ATTEMPT_TIMEOUT - ); + second = await runOnceAndAwaitOutput(NOTEBOOK_FILE_NAME, REFRESHED_OUTPUT, REFRESH_ATTEMPT_TIMEOUT); @@ - expect(second).to.contain('refreshed-host.example.com'); + expect(second).to.contain(REFRESHED_OUTPUT);🤖 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 `@test/e2e/suite/integrationsEnvFileInjection.e2e.test.ts` around lines 117 - 140, Move REFRESH_MAX_ATTEMPTS and REFRESH_ATTEMPT_TIMEOUT to module scope near the other test constants, and define a module-level constant for refreshed-host.example.com. Update the .env write, runOnceAndAwaitOutput call, and final assertion in the live-refresh test to reuse that host constant while preserving the existing retry behavior.Source: Coding guidelines
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
932-963: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for the non-configurable-type guard.
addToProjectIntegrationsskips persistence whenselected.typefailsisConfigurableDatabaseIntegrationType(sqlCellStatusBarProvider.ts Lines 357-360), but no test here exercises a file-only integration with an unconfigurable/unknown type to confirmupdateProjectIntegrationsis not called in that case.🤖 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 `@src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts` around lines 932 - 963, Add a unit test alongside the existing file-only integration test that selects an integration whose type is rejected by isConfigurableDatabaseIntegrationType, then assert addToProjectIntegrations does not call commandNotebookManager.updateProjectIntegrations. Reuse the existing mocked picker and file-based configuration setup, while preserving the current configurable-type test behavior.src/notebooks/deepnote/integrations/integrationManager.ts (1)
116-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
integrationsis never reassigned —constfits.Only mutated via
.set();prefer-constwill likely complain.♻️ Nit
- let integrations = await this.integrationDetector.detectIntegrations({ + const integrations = await this.integrationDetector.detectIntegrations({🤖 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 `@src/notebooks/deepnote/integrations/integrationManager.ts` around lines 116 - 120, Change the integrations declaration in the integration detection flow to const, keeping the existing .set() mutations and all subsequent behavior unchanged.src/notebooks/deepnote/integrations/userpodApiEndpoints.node.ts (1)
29-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePublic accessor/method order isn't alphabetical.
Order here is
baseUrl,ready,getAuthToken,activate; alphabetical would beactivate,baseUrl,getAuthToken,ready. Private fields and private methods elsewhere in the class are correctly alphabetized — just this public group drifted.As per coding guidelines: "Order method, fields and properties, first by accessibility and then by alphabetical order."
🤖 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 `@src/notebooks/deepnote/integrations/userpodApiEndpoints.node.ts` around lines 29 - 52, Reorder the public members in the class alphabetically by name, placing activate before baseUrl, getAuthToken, and ready. Do not change their implementations or the ordering of private members.Source: Coding guidelines
🤖 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 `@specs/INTEGRATIONS_CREDENTIALS.md`:
- Around line 413-416: Update the integration documentation to describe
integrations configured only in .deepnote.env.yaml as file-configured and
read-only, rather than “Not Configured.” State that their merged configuration
remains available to kernel execution and SQL autocomplete, while SecretStorage
edits cannot override the file-owned configuration; apply the same correction to
the related section around the panel labeling behavior.
In `@specs/KERNEL_RESTART_ON_INTEGRATION_CHANGE.md`:
- Line 41: Update the ASCII diagram in KERNEL_RESTART_ON_INTEGRATION_CHANGE.md
to remove the obsolete SqlIntegrationStartupCodeProvider reference and show
deepnote-toolkit fetching credentials from the extension’s loopback userpod-api
endpoint during kernel restart; keep the surrounding restart flow unchanged.
In `@src/notebooks/deepnote/integrations/integrationsEnvFileWatcher.node.ts`:
- Around line 68-74: Extract the shared `deepnote.integrations.envFile.enabled`
configuration check into a reusable helper, then update both
`IntegrationsEnvFileWatcher` and `IntegrationsFileConfigProvider` to use it.
Preserve the existing workspace/file-URI scoping and default value, and keep the
watcher’s disabled-feature behavior unchanged.
- Around line 58-98: Wrap each notebook’s evaluation in findAffectedNotebooks
with a per-iteration try/catch, including hasIntegrationsFile and related
workspace/configuration lookups. On failure, log the notebook-specific error and
continue evaluating the remaining notebooks; preserve the existing filtering and
affected.push behavior for successful iterations.
In `@src/notebooks/deepnote/integrations/userpodApiEndpoints.node.ts`:
- Around line 93-101: Update promptToRecover to explicitly handle rejections
from both window.showErrorMessage and commands.executeCommand instead of using
void fire-and-forget calls. Add rejection handling that logs the failures
through the repository’s established logging mechanism while preserving the
existing reload behavior when the user selects reloadWindow.
In `@src/notebooks/deepnote/sqlCellStatusBarProvider.ts`:
- Around line 242-258: Wrap the getMergedIntegrationConfigs call in the
status-bar display-name logic with try/catch so failures do not propagate
through createStatusBarItems or provideCellStatusBarItems. Follow the existing
error-handling and fallback behavior used by getSelectableIntegrations, while
preserving the current fileConfig lookup and project-integration fallback when
loading succeeds.
In `@src/platform/notebooks/deepnote/integrationsFileConfigProvider.node.ts`:
- Around line 49-61: Update the early returns in the integration configuration
flow after the enabled check and missing-file check to clear diagnostics for
every URI derived from candidateDirs before returning. Reuse the existing
updateDiagnostics mechanism and ensure stale Problems entries are removed both
when integrations.envFile.enabled is false and when the YAML file has been
deleted.
---
Nitpick comments:
In `@src/notebooks/deepnote/integrations/integrationManager.ts`:
- Around line 116-120: Change the integrations declaration in the integration
detection flow to const, keeping the existing .set() mutations and all
subsequent behavior unchanged.
In `@src/notebooks/deepnote/integrations/userpodApiEndpoints.node.ts`:
- Around line 29-52: Reorder the public members in the class alphabetically by
name, placing activate before baseUrl, getAuthToken, and ready. Do not change
their implementations or the ordering of private members.
In `@src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts`:
- Around line 932-963: Add a unit test alongside the existing file-only
integration test that selects an integration whose type is rejected by
isConfigurableDatabaseIntegrationType, then assert addToProjectIntegrations does
not call commandNotebookManager.updateProjectIntegrations. Reuse the existing
mocked picker and file-based configuration setup, while preserving the current
configurable-type test behavior.
In
`@src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts`:
- Around line 103-104: In the provider flow around loadFileIntegrationConfigs
and mergeIntegrationConfigs, replace the duplicated project/notebook/resource
resolution and merge sequence with a call to getMergedIntegrationConfigs.
Preserve the existing trace logging, and remove only the redundant resolution
logic so the shared helper remains the single implementation of this chain.
In `@test/e2e/suite/integrationsEnvFileInjection.e2e.test.ts`:
- Around line 117-140: Move REFRESH_MAX_ATTEMPTS and REFRESH_ATTEMPT_TIMEOUT to
module scope near the other test constants, and define a module-level constant
for refreshed-host.example.com. Update the .env write, runOnceAndAwaitOutput
call, and final assertion in the live-refresh test to reuse that host constant
while preserving the existing retry behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 1427151d-8c7d-496d-bc0e-c90fb0f8da7a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
cspell.jsonpackage.jsonspecs/INTEGRATIONS_CREDENTIALS.mdspecs/KERNEL_RESTART_ON_INTEGRATION_CHANGE.mdsrc/kernels/deepnote/deepnoteIntegrationEndpointEnv.tssrc/kernels/deepnote/deepnoteIntegrationEndpointEnv.unit.test.tssrc/kernels/deepnote/deepnoteLspClientManager.node.tssrc/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.tssrc/kernels/deepnote/sqlLspConnectionUtils.tssrc/kernels/deepnote/sqlLspConnectionUtils.unit.test.tssrc/kernels/execution/cellExecution.federatedAuth.unit.test.tssrc/kernels/execution/cellExecution.tssrc/kernels/jupyter/session/jupyterKernelService.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/serviceRegistry.web.tssrc/messageTypes.tssrc/notebooks/deepnote/deepnoteTestHelpers.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.unit.test.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.unit.test.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.unit.test.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.tssrc/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.unit.test.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationEnvLiveRefresher.node.tssrc/notebooks/deepnote/integrations/integrationEnvLiveRefresher.node.unit.test.tssrc/notebooks/deepnote/integrations/integrationEnvRefreshHandler.tssrc/notebooks/deepnote/integrations/integrationEnvRefreshHandler.unit.test.tssrc/notebooks/deepnote/integrations/integrationKernelRestartHandler.tssrc/notebooks/deepnote/integrations/integrationKernelRestartHandler.unit.test.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationWebview.unit.test.tssrc/notebooks/deepnote/integrations/integrationsEnvFileWatcher.node.tssrc/notebooks/deepnote/integrations/integrationsEnvFileWatcher.node.unit.test.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/userpodApiEndpoints.node.tssrc/notebooks/deepnote/integrations/userpodApiEndpoints.node.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/serviceRegistry.web.tssrc/platform/common/utils/localize.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/integrationsFileConfigProvider.node.tssrc/platform/notebooks/deepnote/integrationsFileConfigProvider.node.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.web.tssrc/platform/notebooks/deepnote/types.tssrc/test/mocks/vscodeFs.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/types.tstest/e2e/fixtures/integrations-env-file.deepnotetest/e2e/suite/integrationsEnvFileInjection.e2e.test.ts
💤 Files with no reviewable changes (8)
- src/notebooks/deepnote/integrations/integrationKernelRestartHandler.unit.test.ts
- src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.unit.test.ts
- src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.ts
- src/notebooks/deepnote/integrations/integrationKernelRestartHandler.ts
- src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
- src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.ts
- src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.unit.test.ts
- src/notebooks/serviceRegistry.web.ts
INTEGRATIONS_CREDENTIALS.md still described file-configured integrations as reading "Not Configured" in the panel, and claimed the panel has no visibility into the merged configs. Both are stale: IntegrationWebviewProvider passes an `isFileConfigured` flag (ids only) and IntegrationItem renders those rows as "Configured in file" with connected styling and the write actions hidden. Also records the detector's file-only append step. KERNEL_RESTART_ON_INTEGRATION_CHANGE.md documented IntegrationKernelRestartHandler and SqlIntegrationStartupCodeProvider, both deleted in this branch — the spec was the only remaining reference to either. Rewritten for the live-refresh flow (no kernel restart) and renamed to INTEGRATION_ENV_LIVE_REFRESH.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
The `deepnote.integrations.envFile.enabled` flag was read in two places that had already drifted: the config provider tested `!enabled`, the watcher `enabled === false`. For a hand-edited non-boolean value those disagree — the provider would disable the feature while the watcher kept firing hidden kernel executions. Extracted `isIntegrationsEnvFileEnabled()` and pointed both at it. Also clear diagnostics before both early returns in `getConfigsForFile`. Nothing below them republishes, so a warning from an earlier read stayed pinned in Problems after the YAML was deleted or the feature turned off. The provider is uncached and re-invoked on every SQL cell execution and panel refresh, so the stale entry survived until a window reload. The watcher suite relied on the shared VS Code mock's `get()`, which ignores the default argument the real API returns when a setting is unset; its stub now honours it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@specs/INTEGRATION_ENV_LIVE_REFRESH.md`:
- Line 76: Specify the fenced code block language in
INTEGRATION_ENV_LIVE_REFRESH.md by changing the ASCII flow diagram’s opening
fence to use the text language identifier. Preserve the diagram content and
closing fence unchanged.
In `@src/platform/notebooks/deepnote/integrationsEnvFileSettings.ts`:
- Around line 14-18: Update the return expression in the integrations
environment-file settings check to use strict equality with true, so only an
actual boolean true enables the feature and values such as "false" are disabled.
🪄 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: CHILL
Plan: Pro
Run ID: 907e55a7-6321-4906-8866-b824de62e336
📒 Files selected for processing (8)
specs/INTEGRATIONS_CREDENTIALS.mdspecs/INTEGRATION_ENV_LIVE_REFRESH.mdspecs/KERNEL_RESTART_ON_INTEGRATION_CHANGE.mdsrc/notebooks/deepnote/integrations/integrationsEnvFileWatcher.node.tssrc/notebooks/deepnote/integrations/integrationsEnvFileWatcher.node.unit.test.tssrc/platform/notebooks/deepnote/integrationsEnvFileSettings.tssrc/platform/notebooks/deepnote/integrationsFileConfigProvider.node.tssrc/platform/notebooks/deepnote/integrationsFileConfigProvider.node.unit.test.ts
💤 Files with no reviewable changes (1)
- specs/KERNEL_RESTART_ON_INTEGRATION_CHANGE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/INTEGRATIONS_CREDENTIALS.md
…tension does
The helper coerced with `Boolean(enabled)` and claimed in its docstring that a
non-boolean value reads as disabled. It doesn't: `Boolean("false")` is `true`, so
only falsy garbage was disabled.
Rather than swap in `enabled === true`, follow the convention already used for
every other boolean setting here — trust the `"type": "boolean"` declared in
package.json and return the value directly, as `snapshots.enabled` and
`enableKernelCompletions` do. VS Code's own built-in extensions read booleans the
same way; where they do guard, they use `typeof x === 'boolean'` rather than
`=== true`, which conflates a malformed value with an explicit false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DqJ6H1EwctFsTT3qcYipdX
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
1 participant
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.
Summary
Adds CLI-parity
.deepnote.env.yaml+.envintegration loading to the extension as a complementary source alongside VSCode SecretStorage, and applies integration env to running kernels the same way Deepnote cloud does — via the toolkit's liveset_integration_env(), with no server restart.What it does
1. File-based integration config (loader + merge)
IntegrationsFileConfigProvider: reads a.deepnote.env.yaml(dir-then-root), resolvesenv:refs against.env(dotenv) andprocess.env(real env wins), never throws. Reuses@deepnote/database-integrations'parseIntegrations; replicates only the Node fs/dotenv shell.SqlIntegrationEnvironmentVariablesProvidermerges file configs over SecretStorage (file wins on id conflict; file-only additive; federated skip + DuckDB unchanged).dotenvdependency and thedeepnote.integrations.envFile.enabledsetting (default true).2. Live env injection (cloud-parity, no restart)
Rather than restarting the toolkit server when integration env changes, the extension mirrors the cloud "no restart" path:
IntegrationsEnvVarsEndpoint: a loopback HTTP endpoint (127.0.0.1, no auth) servingGET /userpod-api/:projectId/integrations/environment-variables→[{name,value}]from the provider. This is exactly what the toolkit'sset_integration_env()fetches.DEEPNOTE_RUNTIME__ENV_INTEGRATION_ENABLED/__RUNNING_IN_DETACHED_MODE/__WEBAPP_URL+DEEPNOTE_PROJECT_ID— so it fetches integration env at kernel start. Guarded to degrade to the existing spawn-time injection when the endpoint/project id isn't available.IntegrationEnvLiveRefresher: on an env-file change (IntegrationsEnvFileWatcher, debounced) or a SecretStorage integration change (IntegrationEnvRefreshHandler), runsdeepnote_toolkit.set_integration_env()silently (executeHidden) in the affected running kernels — re-fetch + liveos.environupdate — and shows one dismissible "environment updated" notification.The spawn-time SQL_* injection is retained as the initial-load safety net. Node-only feature; web is unchanged.
Test plan
integrationsEnvFileInjection.e2e.test.tsproves the.deepnote.env.yaml→env:→ dotenv → integration → kernel path via spawn-time injection. Runs under ExTester in CI.set_integration_env()loop — the toolkit fetching the local endpoint and updating a running kernel'sos.environon change.Follow-up (separate
deepnote/deepnoterepo)@deepnote/database-integrationscould add a Node-only subpath export (/node) for the fs/dotenv shell and aDatabaseIntegrationConfig-typed federated guard, to remove the small pieces the extension/CLI currently replicate (plan drafted separately).🤖 Generated with Claude Code
https://claude.ai/code/session_01CPTs6CHNncauUuTGkwpNtH
Summary by CodeRabbit
New Features
.deepnote.env.yaml, with values resolved from.env.Bug Fixes