feat(integrations): Add support for federated auth integrations, starting with BigQuery

[tkislan]

Summary by CodeRabbit

• New Features

  • Google OAuth for BigQuery integrations with Authenticate/Re-authenticate actions and per-integration token status in the integrations panel.
  • BigQuery SQL blocks may run using stored OAuth tokens (queries execute with refreshed access tokens).
  • Kernels automatically restart when relevant integration tokens change; orphaned tokens are pruned.
  • BigQuery form supports both service-account and OAuth modes; Deepnote domain now resolves per-resource.

• Bug Fixes

  • Web shows a clear “not supported in web” message for federated auth flows.

• Tests

  • Extensive unit coverage for federated auth flows, token storage, OAuth loopback, codegen, and related helpers.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements federated authentication for BigQuery integrations in the VS Code Deepnote extension. It introduces OAuth-based token management with refresh logic, a loopback browser flow for acquiring credentials, SQL code generation that embeds fresh access tokens, and extension services that manage token lifecycle. The webview now displays token status and supports re-authentication. Cell execution conditionally runs a silent prelude to set connection context before executing federated SQL blocks. Kernel restarts are triggered when tokens change, and stale tokens are pruned when integrations are removed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (862f6e9) to head (165b7e5).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #399   +/-   ##
===========================
===========================

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 9

🤖 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 `@src/kernels/execution/cellExecution.federatedAuth.unit.test.ts`:
- Around line 217-225: Replace the multiple individual property assertions on
the first call with a single assert.deepStrictEqual comparing the full request
payload: build an expected object for the prelude (e.g., { code: prelude,
silent: true, store_history: false, allow_stdin: false, stop_on_error: true })
and call assert.deepStrictEqual(preludeContent, expectedPrelude); keep the
existing boolean check for preludeDispose if desired or include it in a second
deepStrictEqual against the full args tuple; do the same refactor for the second
call (calls[1] / its requestArgs/requestContent) referenced in the comment so
each payload comparison uses a single assert.deepStrictEqual rather than many
assert.strictEqual calls.

In `@src/kernels/execution/cellExecution.ts`:
- Around line 486-500: The current await of
kernelConnection.requestExecute(...).done ignores execute_reply content.status
so errors inside the federated prelude don't throw; change the call to capture
the reply (e.g., const reply = await
kernelConnection.requestExecute({...}).done), then inspect reply.content?.status
and if it equals "error" call logger.error with context including
federated.prelude and the reply content, and return
this.completedWithErrors(reply) (or an Error created from the reply) to stop
main execution; keep existing try/catch for transport exceptions around the same
kernelConnection.requestExecute call and use federated.prelude,
kernelConnection.requestExecute, this.completedWithErrors, and logger.error to
locate where to implement the check.

In
`@src/notebooks/deepnote/integrations/federatedAuth/federatedAuthInvariants.unit.test.ts`:
- Around line 10-13: The current conditional type distributes over the union
'accessToken' | 'expiresAt' causing a false negative; update the invariant to
test the union as a whole by using a non-distributive or aggregate check (for
example, use Extract<keyof FederatedAuthTokenEntry, Forbidden> extends never ?
true : never or wrap Forbidden in a tuple like [Forbidden] extends [keyof
FederatedAuthTokenEntry] ?) so that the test fails if any forbidden key is
present; change the AssertEntryOmitsForbidden definition that references
FederatedAuthTokenEntry and Forbidden accordingly and keep the rest of the
assertion (_entryShapeCheck and void usage) intact.

In
`@src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.unit.test.ts`:
- Line 83: Replace the repeated magic-number waits by extracting a named
constant and helper: add a top-of-module constant (e.g., const TEST_WAIT_MS =
10) and a small async helper function (e.g., async function waitMs(ms =
TEST_WAIT_MS) { return new Promise(resolve => setTimeout(resolve, ms)); }) and
then replace every occurrence of await new Promise((resolve) =>
setTimeout(resolve, 10)); in the tests with await waitMs(); so fixtures and
tests (references in federatedAuthKernelRestartBridge.node.unit.test.ts) use the
shared constant and helper to avoid duplication.

In
`@src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.ts`:
- Around line 31-33: In FederatedAuthOrphanedTokenCleaner, the activate() method
is currently a no-op so any orphaned tokens present at startup are never
cleaned; update activate() to invoke the existing orphaned-token cleanup routine
used for integration-change events (call the exact method name in this class —
e.g., cleanOrphanedTokens(), runOrphanedTokenCleanup(), or similar) so cleanup
runs once on activation, await or handle the returned Promise if the cleanup is
async, and catch/log any errors so activation doesn't crash.

In `@src/notebooks/deepnote/integrations/integrationWebview.ts`:
- Around line 665-666: When deleting an integration, token cleanup
(tokenStorage.delete(integrationId)) must not be allowed to abort the overall
delete/reset flow if it fails; wrap the tokenStorage.delete(integrationId) calls
(the ones at/around where integrationStorage.delete(...) is called in
integrationWebview.ts) in a try/catch, log the error (or call process/project
logger) inside the catch, and do not rethrow so the in-memory and project state
transitions (the integrationStorage.delete and subsequent state updates) always
complete; apply the same pattern to both occurrences (the calls at the two
locations noted).
- Around line 50-52: The event callback passed to tokenStorage.onDidChangeTokens
calls this.updateWebview() without handling rejections, which can produce
unhandled promise rejections; change the callback so it consumes errors from
updateWebview (for example replace void this.updateWebview() with
this.updateWebview().catch(err => { /* handle or log the error */ }) or wrap the
call in an async IIFE with try/catch) so any rejection from updateWebview is
caught; reference tokenStorage.onDidChangeTokens and the updateWebview method
when making the change.

In
`@src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts`:
- Around line 110-116: The Promise.all call loading integration configs will
abort all work if any getIntegrationConfig rejects; change to per-item error
isolation by using Promise.allSettled over projectIntegrations.map(...) (or wrap
each await in try/catch inside the map) to collect only fulfilled results into
allConfigs, filter out rejected/undefined entries, and log the individual errors
(including integration.id) so one failing getIntegrationConfig call in
integrationStorage.getIntegrationConfig does not prevent returning other env
vars (including DuckDB).

In `@src/webviews/webview-side/integrations/BigQueryForm.tsx`:
- Around line 44-55: buildInitialConfig currently returns a blind clone of
existingConfig which can carry an unsupported authMethod into the form; change
it to clone the object (structuredClone(existingConfig)) then validate
existingConfig.authMethod against the supported set (e.g., 'service-account' and
'google-oauth') and if it's not supported, normalize it to a safe default (for
example 'service-account') and clear or reset any auth-specific fields (e.g.,
serviceAccountKey, oauthToken, oauthClientId/secrets) so the form doesn't start
in an invalid state; use the function buildInitialConfig and
createEmptyBigQueryConfig as anchors for where to perform this normalization.

🪄 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: 36bbea8e-d36e-4a6d-b50b-3de8abb8e3e5

📥 Commits

Reviewing files that changed from the base of the PR and between 8efe0ed and 4da300a.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (41)

• package.json
• package.nls.json
• src/kernels/execution/cellExecution.federatedAuth.unit.test.ts
• src/kernels/execution/cellExecution.ts
• src/kernels/kernelExecution.ts
• src/kernels/kernelProvider.node.ts
• src/messageTypes.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.web.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.web.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthInvariants.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/googleOAuthProvider.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/googleOAuthProvider.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/oauthLoopbackFlow.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/oauthLoopbackFlow.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts
• src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.unit.test.ts
• src/notebooks/deepnote/integrations/integrationWebview.ts
• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/notebooks/deepnote/integrations/types.ts
• src/notebooks/serviceRegistry.node.ts
• src/notebooks/serviceRegistry.web.ts
• src/platform/common/constants.ts
• src/platform/common/utils/localize.ts
• src/platform/notebooks/deepnote/integrationTypes.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
• src/webviews/webview-side/integrations/BigQueryForm.tsx
• src/webviews/webview-side/integrations/IntegrationItem.tsx
• src/webviews/webview-side/integrations/IntegrationList.tsx
• src/webviews/webview-side/integrations/IntegrationPanel.tsx
• src/webviews/webview-side/integrations/types.ts

[tkislan]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/webviews/webview-side/integrations/IntegrationItem.tsx (1)

8-14: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Keep props alphabetically ordered after adding onAuthenticate.

onAuthenticate was appended, but this interface/destructuring block should stay alphabetized to match repo ordering rules.

Proposed reorder

 export interface IIntegrationItemProps {
     integration: IntegrationWithStatus;
+    onAuthenticate: (integrationId: string) => void;
     onConfigure: (integrationId: string) => void;
-    onReset: (integrationId: string) => void;
     onDelete: (integrationId: string) => void;
-    onAuthenticate: (integrationId: string) => void;
+    onReset: (integrationId: string) => void;
 }

 export const IntegrationItem: React.FC<IIntegrationItemProps> = ({
     integration,
+    onAuthenticate,
     onConfigure,
-    onReset,
     onDelete,
-    onAuthenticate
+    onReset
 }) => {

As per coding guidelines: "Order method, fields and properties, first by accessibility and then by alphabetical order".

Also applies to: 57-63

🤖 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/webviews/webview-side/integrations/IntegrationItem.tsx` around lines 8 -
14, The IIntegrationItemProps interface and the props destructuring in the
IntegrationItem component must be alphabetized: move onAuthenticate into the
correct alphabetical position among integration, onConfigure, onDelete, onReset,
etc., and reorder the corresponding destructured props in the IntegrationItem
function to match; update both the IIntegrationItemProps declaration and the
destructuring usage so property order is alphabetical (e.g., integration,
onAuthenticate, onConfigure, onDelete, onReset).

🤖 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 `@src/kernels/errors/cellExecutionOutputError.ts`:
- Around line 1-2: This new file contains the Microsoft copyright header at the
top; remove the header lines so the file only contains the TypeScript code
(i.e., delete the leading comment block in
src/kernels/errors/cellExecutionOutputError.ts), leaving the existing exported
symbol(s) like CellExecutionOutputError and any imports/exports intact.

In `@src/kernels/execution/cellExecution.federatedAuth.unit.test.ts`:
- Around line 102-106: Test setup creates a CancellationTokenSource named
tokenSource and pushes it to disposables but it is never used or disposed;
either remove the unused tokenSource and its push from the setup block, or use
it in the tests (e.g., pass tokenSource.token to functions under test and call
tokenSource.cancel() in a test) and ensure you call tokenSource.dispose() (or
remove from disposables cleanup if you keep it) so CancellationTokenSource is
actually exercised and properly disposed; locate the tokenSource variable in the
setup function and update either its creation/push or add usage and disposal
accordingly.

In `@src/messageTypes.ts`:
- Around line 231-250: The block of new LocalizedMessages keys for BigQuery
federated-auth is not alphabetized; reorder the keys within the BigQuery
federated-auth section so they follow alphabetical order by property name (e.g.,
place integrationsAuthenticate, integrationsAuthenticating,
integrationsAuthenticationFailed, integrationsAuthenticationSucceeded,
integrationsBigQueryAuthMethodGoogleOauth, integrationsBigQueryAuthMethodLabel,
integrationsBigQueryAuthMethodServiceAccount, integrationsBigQueryClientIdLabel,
integrationsBigQueryClientIdPlaceholder, integrationsBigQueryClientSecretLabel,
integrationsBigQueryClientSecretPlaceholder,
integrationsBigQueryNotAuthenticated, integrationsBigQueryProjectLabel,
integrationsBigQueryProjectPlaceholder,
integrationsFederatedAuthNotSupportedInWeb, integrationsReauthenticate,
integrationsTokenStatusAuthenticated, integrationsTokenStatusDisconnected);
update the sequence around the identifiers mentioned
(integrationsBigQueryAuthMethodLabel,
integrationsBigQueryAuthMethodServiceAccount,
integrationsBigQueryAuthMethodGoogleOauth, integrationsBigQueryProjectLabel,
integrationsBigQueryProjectPlaceholder, integrationsBigQueryClientIdLabel,
integrationsBigQueryClientIdPlaceholder, integrationsBigQueryClientSecretLabel,
integrationsBigQueryClientSecretPlaceholder, integrationsAuthenticate,
integrationsReauthenticate, integrationsTokenStatusAuthenticated,
integrationsTokenStatusDisconnected, integrationsAuthenticating,
integrationsAuthenticationSucceeded, integrationsAuthenticationFailed,
integrationsBigQueryNotAuthenticated,
integrationsFederatedAuthNotSupportedInWeb) so the section follows alphabetical
ordering.

In `@src/notebooks/controllers/vscodeNotebookController.ts`:
- Around line 670-674: The catch blocks currently reassign the catch parameter
ex using WrappedError.unwrap(ex), which violates noCatchAssign; instead create a
new local (e.g., const unwrapped = WrappedError.unwrap(ex)) and use that for
subsequent instanceof checks and logging/rendering (replace uses of the
reassigned ex with unwrapped in the block where you check for
CellExecutionOutputError and any error rendering code). Ensure you do this for
all catch blocks in vscodeNotebookController.ts that call WrappedError.unwrap on
the catch parameter.

In
`@src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTestHelpers.ts`:
- Around line 128-130: Introduce a top-of-module named constant (e.g.,
SETTLE_ASYNC_HANDLERS_DELAY_MS) and use it as the default timeout for
settleAsyncHandlers instead of the magic number 10; update the
settleAsyncHandlers function signature to default ms to that constant and
replace any inline uses of 10 for this helper with the new constant so the
intent and configuration are centralized.

In `@src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts`:
- Around line 32-34: The escapePythonString function fails to escape carriage
returns; update escapePythonString to also replace '\r' with '\\r' (ensuring you
still first escape backslashes, then single quotes, then newlines and carriage
returns) so generated Python string literals won't break when value contains
'\r'.

In `@src/notebooks/deepnote/integrations/integrationWebview.ts`:
- Around line 36-57: The tokenStorageDisposables array (used to hold the
onDidChangeTokens subscription) is never disposed, causing a leak; add a cleanup
path by implementing a dispose() on the IntegrationWebviewProvider (or register
the provider with IDisposableRegistry) that iterates tokenStorageDisposables and
calls dispose() on each Disposable (and clears the array), and ensure
this.dispose() is invoked when the provider is torn down so the
IFederatedAuthTokenStorage.onDidChangeTokens subscription is unsubscribed.
- Around line 489-521: The tokenStorage.delete calls inside
invalidateStaleFederatedToken can throw and bubble up, breaking the caller's
saveConfiguration; wrap both delete invocations in a safe failure handler
(either a try/catch around the await this.tokenStorage.delete(integrationId)
calls or append .catch(err => { logger.warn(...); })) so deletion errors are
logged but do not reject the surrounding operation; adjust logging to include
the error and continue without rethrowing to ensure saveConfiguration can
proceed.

In
`@src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts`:
- Around line 14-20: Reorder imports so third-party package imports come before
local project imports: move the block importing DatabaseIntegrationConfig,
FederatedAuthMethod, getEnvironmentVariablesForIntegrations, and
isFederatedAuthMethod from '`@deepnote/database-integrations`' above the local
imports (the '../../common/*' and './*' imports) in
sqlIntegrationEnvironmentVariablesProvider (locate the import statements by
those symbol names) to follow the third-party/local grouping guideline.

---

Outside diff comments:
In `@src/webviews/webview-side/integrations/IntegrationItem.tsx`:
- Around line 8-14: The IIntegrationItemProps interface and the props
destructuring in the IntegrationItem component must be alphabetized: move
onAuthenticate into the correct alphabetical position among integration,
onConfigure, onDelete, onReset, etc., and reorder the corresponding destructured
props in the IntegrationItem function to match; update both the
IIntegrationItemProps declaration and the destructuring usage so property order
is alphabetical (e.g., integration, onAuthenticate, onConfigure, onDelete,
onReset).

🪄 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: e109f31b-1a4a-4bbb-9158-99fd3e0b3815

📥 Commits

Reviewing files that changed from the base of the PR and between cf5d53f and cdd519b.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (49)

• cspell.json
• package.json
• package.nls.json
• src/kernels/errors/cellExecutionOutputError.ts
• src/kernels/execution/cellExecution.federatedAuth.unit.test.ts
• src/kernels/execution/cellExecution.ts
• src/kernels/execution/types.ts
• src/kernels/kernelExecution.ts
• src/kernels/kernelProvider.node.ts
• src/messageTypes.ts
• src/notebooks/controllers/vscodeNotebookController.ts
• src/notebooks/deepnote/importClient.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.web.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthCommandHandler.web.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthInvariants.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthOrphanedTokenCleaner.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthSqlBlockCodeGenerator.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTestHelpers.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTokenStorage.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/googleOAuthProvider.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/googleOAuthProvider.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/oauthLoopbackFlow.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/oauthLoopbackFlow.node.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts
• src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.unit.test.ts
• src/notebooks/deepnote/integrations/integrationWebview.ts
• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/notebooks/deepnote/integrations/types.ts
• src/notebooks/deepnote/openInDeepnoteHandler.node.ts
• src/notebooks/serviceRegistry.node.ts
• src/notebooks/serviceRegistry.web.ts
• src/platform/common/constants.ts
• src/platform/common/utils/localize.ts
• src/platform/notebooks/deepnote/integrationTypes.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
• src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
• src/webviews/webview-side/integrations/BigQueryForm.tsx
• src/webviews/webview-side/integrations/IntegrationItem.tsx
• src/webviews/webview-side/integrations/IntegrationList.tsx
• src/webviews/webview-side/integrations/IntegrationPanel.tsx
• src/webviews/webview-side/integrations/types.ts

[coderabbitai]

♻️ Duplicate comments (1)

src/notebooks/deepnote/integrations/integrationWebview.ts (1)

506-527: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract the best-effort token-delete path.

These two branches now duplicate the same delete-and-warn flow already used elsewhere in the class. A small helper will keep cleanup behavior from drifting.

As per coding guidelines: "Extract duplicate logic into helper methods to prevent drift following DRY principle".

🤖 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/integrationWebview.ts` around lines 506 -
527, The two code paths in IntegrationWebviewProvider that call await
this.tokenStorage.delete(integrationId).catch((err) => logger.warn(...)) should
be consolidated into a small helper method (e.g., a private async
handleStaleTokenDelete(integrationId: string, reason?: string)) that performs
the delete and logs the same warn message on error and an optional info log for
the reason; replace both occurrences (the early-delete branch and the
fingerprint-change branch that compares newFingerprint !==
stored.metadataFingerprint) with calls to this helper to centralize the
delete-and-warn behavior.

🤖 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.

Duplicate comments:
In `@src/notebooks/deepnote/integrations/integrationWebview.ts`:
- Around line 506-527: The two code paths in IntegrationWebviewProvider that
call await this.tokenStorage.delete(integrationId).catch((err) =>
logger.warn(...)) should be consolidated into a small helper method (e.g., a
private async handleStaleTokenDelete(integrationId: string, reason?: string))
that performs the delete and logs the same warn message on error and an optional
info log for the reason; replace both occurrences (the early-delete branch and
the fingerprint-change branch that compares newFingerprint !==
stored.metadataFingerprint) with calls to this helper to centralize the
delete-and-warn behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: bccc74c9-0c64-40db-8980-2a0fbe64f38e

📥 Commits

Reviewing files that changed from the base of the PR and between cdd519b and b46bc10.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (7)

• package.json
• src/kernels/errors/cellExecutionOutputError.ts
• src/kernels/execution/cellExecution.federatedAuth.unit.test.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthTestHelpers.ts
• src/notebooks/deepnote/integrations/integrationWebview.ts
• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts

💤 Files with no reviewable changes (2)

• src/kernels/errors/cellExecutionOutputError.ts
• src/kernels/execution/cellExecution.federatedAuth.unit.test.ts

[coderabbitai]

♻️ Duplicate comments (1)

src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts (1)

31-33: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing \r escape remains unaddressed.

Carriage returns still aren't escaped. If value contains \r, generated Python literals may break.

Suggested fix

 export function escapePythonString(value: string): string {
-    return `'${value.replaceAll('\\', '\\\\').replaceAll("'", "\\'").replaceAll('\n', '\\n')}'`;
+    return `'${value.replaceAll('\\', '\\\\').replaceAll("'", "\\'").replaceAll('\n', '\\n').replaceAll('\r', '\\r')}'`;
 }

🤖 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/federatedAuth/vendoredBlocksHelpers.ts`
around lines 31 - 33, The escapePythonString function currently does not escape
carriage returns, which can break generated Python literals; update
escapePythonString to also replace '\r' with '\\r' (e.g., add a
.replaceAll('\r', '\\r') or equivalent step in the replacement chain inside the
function) while keeping the existing backslash, single-quote and newline
replacements intact so the function remains escapePythonString and preserves its
other behaviors.

🤖 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.

Duplicate comments:
In `@src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts`:
- Around line 31-33: The escapePythonString function currently does not escape
carriage returns, which can break generated Python literals; update
escapePythonString to also replace '\r' with '\\r' (e.g., add a
.replaceAll('\r', '\\r') or equivalent step in the replacement chain inside the
function) while keeping the existing backslash, single-quote and newline
replacements intact so the function remains escapePythonString and preserves its
other behaviors.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 09f746a5-46b7-446b-8136-34aa50ac3c7f

📥 Commits

Reviewing files that changed from the base of the PR and between b46bc10 and 165b7e5.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (2)

• src/notebooks/deepnote/integrations/federatedAuth/vendoredBlocksHelpers.ts
• src/notebooks/deepnote/integrations/integrationWebview.ts

💤 Files with no reviewable changes (1)

• src/notebooks/deepnote/integrations/integrationWebview.ts