Enhance/snowflake web browser type connection imlementation - #347
Draft
Nuri1977 wants to merge 2 commits into
Draft
Enhance/snowflake web browser type connection imlementation#347Nuri1977 wants to merge 2 commits into
Nuri1977 wants to merge 2 commits into
Conversation
…tion - Add 'Web Browser' vs 'Password Login' tabs to the Snowflake connection UI - Add new 'Account Locator' field required for browser-based SSO flows - Update `snowflake-sdk` configuration logic to support `EXTERNALBROWSER` authenticator - Update `connectors.service.ts` to generate `authenticator: externalbrowser` in dbt profiles - Wrap Snowflake SDK connection attempts in Promises to properly capture and surface SAML/SSO authentication failures to the frontend - Fix ESLint warnings and errors (nested ternaries, unused variables) - Update implementation plan to mark feature as paused (pending end-to-end testing with a live SAML/SSO Identity Provider)
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/services/cloudExplorer.service.ts (1)
164-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
as anycasts from the sixgetSignedUrlcalls.
@aws-sdk/client-s3and@aws-sdk/s3-request-presignerare already aligned inpackage.json, so these casts only bypass type checking at the presigner boundary.🤖 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/main/services/cloudExplorer.service.ts` around lines 164 - 166, Remove the client and command `as any` casts from all six `getSignedUrl` calls in the cloud explorer service, passing the typed values directly to the presigner while preserving the existing expiration options and return 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.
Inline comments:
In `@src/main/services/connectors.service.ts`:
- Around line 686-691: Wrap the `case 'snowflake'` clause in its own block so
the `result` declaration is scoped within that case and satisfies
`noSwitchDeclarations`; preserve the existing `testSnowflakeConnection`, failure
logging, and return behavior.
- Around line 61-79: Update ConnectorsService.logSnowflakeTestFailure to remove
connection.username and any raw authentication-related fields from the
console.error payload. Retain only controlled, non-sensitive identifiers and the
structured result code/message; avoid exposing SDK or SSO details while
preserving the failure context.
In `@src/main/utils/connectors.ts`:
- Around line 375-377: Destroy the persistent Snowflake client before returning
the connect error in src/main/utils/connectors.ts lines 375-377. Move extractor
disconnection into a finally block in src/main/services/projects.service.ts
lines 998-1005 so cleanup runs on every success and failure path.
- Around line 236-248: Update the error classification around the Snowflake
connector validation branch to handle “incorrect username or password”
separately from invalid account identifier errors. Preserve the existing
account-identifier message for account, connection, and hostname-related
failures, but return an authentication-appropriate message and details for
credential failures.
In `@src/renderer/hooks/useConnectionInput.ts`:
- Line 52: Preserve Snowflake account identity across all listed paths: in
src/renderer/hooks/useConnectionInput.ts lines 52-52, include sf.accountLocator
in the returned input; in src/main/services/connectors.service.ts lines 156-169,
compare effective locators before reusing connections; in connectors.service.ts
lines 1989-1998 and projects.service.ts lines 989-998, pass or resolve the
account as accountLocator || account for browser extraction and related
handling.
---
Nitpick comments:
In `@src/main/services/cloudExplorer.service.ts`:
- Around line 164-166: Remove the client and command `as any` casts from all six
`getSignedUrl` calls in the cloud explorer service, passing the typed values
directly to the presigner while preserving the existing expiration options and
return 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c516ce1-0e0e-41c0-989f-95f682a04596
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/main/extractor/snowflake.extractor.tssrc/main/services/ai/tools/studio/connections.tools.tssrc/main/services/cloudExplorer.service.tssrc/main/services/connectors.service.tssrc/main/services/projects.service.tssrc/main/utils/connectors.tssrc/main/utils/yamlPartialUpdate.tssrc/renderer/components/connections/bigquery.tsxsrc/renderer/components/connections/snowflake.tsxsrc/renderer/controllers/connectors.controller.tssrc/renderer/hooks/useConnectionInput.tssrc/renderer/services/connectors.service.tssrc/types/backend.ts
Comment on lines
+61
to
+79
| private static logSnowflakeTestFailure( | ||
| connection: SnowflakeConnection, | ||
| result: ConnectionTestResult, | ||
| ): void { | ||
| // eslint-disable-next-line no-console | ||
| console.error('[ConnectorsService] Snowflake test connection failed', { | ||
| scope: 'connectors', | ||
| provider: 'snowflake', | ||
| operation: 'testConnection', | ||
| authMethod: this.getSnowflakeAuthMethod(connection), | ||
| account: connection.account, | ||
| username: connection.username, | ||
| warehouse: connection.warehouse, | ||
| database: connection.database, | ||
| schema: connection.schema, | ||
| code: result.code, | ||
| message: result.message, | ||
| details: result.details, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log Snowflake usernames or raw authentication details.
Usernames may be email addresses, while SDK/SSO details can contain sensitive identity-provider information. Log controlled codes/messages instead.
Proposed adjustment
authMethod: this.getSnowflakeAuthMethod(connection),
account: connection.account,
- username: connection.username,
+ usernameProvided: Boolean(connection.username),
warehouse: connection.warehouse,
database: connection.database,
schema: connection.schema,
code: result.code,
message: result.message,
- details: result.details,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static logSnowflakeTestFailure( | |
| connection: SnowflakeConnection, | |
| result: ConnectionTestResult, | |
| ): void { | |
| // eslint-disable-next-line no-console | |
| console.error('[ConnectorsService] Snowflake test connection failed', { | |
| scope: 'connectors', | |
| provider: 'snowflake', | |
| operation: 'testConnection', | |
| authMethod: this.getSnowflakeAuthMethod(connection), | |
| account: connection.account, | |
| username: connection.username, | |
| warehouse: connection.warehouse, | |
| database: connection.database, | |
| schema: connection.schema, | |
| code: result.code, | |
| message: result.message, | |
| details: result.details, | |
| }); | |
| private static logSnowflakeTestFailure( | |
| connection: SnowflakeConnection, | |
| result: ConnectionTestResult, | |
| ): void { | |
| // eslint-disable-next-line no-console | |
| console.error('[ConnectorsService] Snowflake test connection failed', { | |
| scope: 'connectors', | |
| provider: 'snowflake', | |
| operation: 'testConnection', | |
| authMethod: this.getSnowflakeAuthMethod(connection), | |
| account: connection.account, | |
| usernameProvided: Boolean(connection.username), | |
| warehouse: connection.warehouse, | |
| database: connection.database, | |
| schema: connection.schema, | |
| code: result.code, | |
| message: result.message, | |
| }); |
🤖 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/main/services/connectors.service.ts` around lines 61 - 79, Update
ConnectorsService.logSnowflakeTestFailure to remove connection.username and any
raw authentication-related fields from the console.error payload. Retain only
controlled, non-sensitive identifiers and the structured result code/message;
avoid exposing SDK or SSO details while preserving the failure context.
Comment on lines
686
to
+691
| case 'snowflake': | ||
| try { | ||
| return await testSnowflakeConnection(connection); | ||
| } catch { | ||
| return false; | ||
| const result = await testSnowflakeConnection(connection); | ||
| if (!result.ok) { | ||
| this.logSnowflakeTestFailure(connection, result); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Wrap the Snowflake switch clause in a block.
The lexical result declaration violates noSwitchDeclarations and can fail the lint pipeline.
Proposed fix
- case 'snowflake':
+ case 'snowflake': {
const result = await testSnowflakeConnection(connection);
if (!result.ok) {
this.logSnowflakeTestFailure(connection, result);
}
return result;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case 'snowflake': | |
| try { | |
| return await testSnowflakeConnection(connection); | |
| } catch { | |
| return false; | |
| const result = await testSnowflakeConnection(connection); | |
| if (!result.ok) { | |
| this.logSnowflakeTestFailure(connection, result); | |
| } | |
| return result; | |
| case 'snowflake': { | |
| const result = await testSnowflakeConnection(connection); | |
| if (!result.ok) { | |
| this.logSnowflakeTestFailure(connection, result); | |
| } | |
| return result; | |
| } |
🧰 Tools
🪛 Biome (2.5.3)
[error] 687-687: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 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/main/services/connectors.service.ts` around lines 686 - 691, Wrap the
`case 'snowflake'` clause in its own block so the `result` declaration is scoped
within that case and satisfies `noSwitchDeclarations`; preserve the existing
`testSnowflakeConnection`, failure logging, and return behavior.
Source: Linters/SAST tools
Comment on lines
+236
to
+248
| if ( | ||
| lowercaseMessage.includes('account') || | ||
| lowercaseMessage.includes('incorrect username or password') || | ||
| lowercaseMessage.includes('could not connect') || | ||
| lowercaseMessage.includes('nodename nor servname provided') | ||
| ) { | ||
| return { | ||
| ok: false, | ||
| code, | ||
| message: 'Snowflake account validation failed', | ||
| details: 'The Snowflake account identifier appears invalid.', | ||
| authFlow, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not report invalid credentials as an invalid account identifier.
“Incorrect username or password” currently returns “account identifier appears invalid,” directing users toward the wrong fix.
Proposed fix
+ if (lowercaseMessage.includes('incorrect username or password')) {
+ return {
+ ok: false,
+ code,
+ message: 'Snowflake authentication failed',
+ details: 'Check the configured Snowflake username and password.',
+ authFlow,
+ };
+ }
+
if (
lowercaseMessage.includes('account') ||
- lowercaseMessage.includes('incorrect username or password') ||
lowercaseMessage.includes('could not connect') ||📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| lowercaseMessage.includes('account') || | |
| lowercaseMessage.includes('incorrect username or password') || | |
| lowercaseMessage.includes('could not connect') || | |
| lowercaseMessage.includes('nodename nor servname provided') | |
| ) { | |
| return { | |
| ok: false, | |
| code, | |
| message: 'Snowflake account validation failed', | |
| details: 'The Snowflake account identifier appears invalid.', | |
| authFlow, | |
| }; | |
| if (lowercaseMessage.includes('incorrect username or password')) { | |
| return { | |
| ok: false, | |
| code, | |
| message: 'Snowflake authentication failed', | |
| details: 'Check the configured Snowflake username and password.', | |
| authFlow, | |
| }; | |
| } | |
| if ( | |
| lowercaseMessage.includes('account') || | |
| lowercaseMessage.includes('could not connect') || | |
| lowercaseMessage.includes('nodename nor servname provided') | |
| ) { | |
| return { | |
| ok: false, | |
| code, | |
| message: 'Snowflake account validation failed', | |
| details: 'The Snowflake account identifier appears invalid.', | |
| authFlow, | |
| }; | |
| } |
🤖 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/main/utils/connectors.ts` around lines 236 - 248, Update the error
classification around the Snowflake connector validation branch to handle
“incorrect username or password” separately from invalid account identifier
errors. Preserve the existing account-identifier message for account,
connection, and hostname-related failures, but return an
authentication-appropriate message and details for credential failures.
Comment on lines
+375
to
+377
| } catch (err: any) { | ||
| return { success: false, error: err.message }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Destroy persistent Snowflake clients on every failure path.
src/main/utils/connectors.ts#L375-L377: destroy the connection before returning a connect error.src/main/services/projects.service.ts#L998-L1005: move extractor disconnection intofinally.
Based on learnings, persistent Snowflake connections require explicit cleanup.
📍 Affects 2 files
src/main/utils/connectors.ts#L375-L377(this comment)src/main/services/projects.service.ts#L998-L1005
🤖 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/main/utils/connectors.ts` around lines 375 - 377, Destroy the persistent
Snowflake client before returning the connect error in
src/main/utils/connectors.ts lines 375-377. Move extractor disconnection into a
finally block in src/main/services/projects.service.ts lines 998-1005 so cleanup
runs on every success and failure path.
Source: Learnings
| warehouse: sf.warehouse, | ||
| schema: sf.schema || 'PUBLIC', | ||
| role: sf.role, | ||
| authMethod: sf.authMethod, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the Snowflake browser account identity end-to-end.
authMethod is propagated, but accountLocator is dropped or ignored in several paths:
src/renderer/hooks/useConnectionInput.ts#L52-L52: includesf.accountLocatorin the returned input.src/main/services/connectors.service.ts#L156-L169: compare effective account locators before reusing connections.src/main/services/connectors.service.ts#L1989-L1998: passsfConn.accountLocator || sfConn.accountfor browser extraction.src/main/services/projects.service.ts#L989-L998: use the same effective account resolution.
📍 Affects 3 files
src/renderer/hooks/useConnectionInput.ts#L52-L52(this comment)src/main/services/connectors.service.ts#L156-L169src/main/services/connectors.service.ts#L1989-L1998src/main/services/projects.service.ts#L989-L998
🤖 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/renderer/hooks/useConnectionInput.ts` at line 52, Preserve Snowflake
account identity across all listed paths: in
src/renderer/hooks/useConnectionInput.ts lines 52-52, include sf.accountLocator
in the returned input; in src/main/services/connectors.service.ts lines 156-169,
compare effective locators before reusing connections; in connectors.service.ts
lines 1989-1998 and projects.service.ts lines 989-998, pass or resolve the
account as accountLocator || account for browser extraction and related
handling.
Nuri1977
marked this pull request as draft
July 16, 2026 16:16
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 by CodeRabbit