feat(gsheets): restore public/private sheet selector#40466
Open
kgabryje wants to merge 8 commits into
Open
Conversation
dosubot
Bot
added
change:frontend
Contributor
Code Review Agent Run #62f398Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40466 +/- ##
==========================================
+ Coverage 64.05% 64.07% +0.02%
==========================================
Files 2593 2593
Lines 139693 139757 +64
Branches 32446 32477 +31
==========================================
+ Hits 89479 89551 +72
+ Misses 48676 48668 -8
Partials 1538 1538
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Code Review Agent Run #49bbe5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+51
to
+67
| const deriveOauth2ClientInfo = (): OAuth2ClientInfo => { | ||
| const encryptedExtra = JSON.parse(db?.masked_encrypted_extra || '{}'); | ||
| return { | ||
| id: encryptedExtra.oauth2_client_info?.id || '', | ||
| secret: encryptedExtra.oauth2_client_info?.secret || '', | ||
| authorization_request_uri: | ||
| encryptedExtra.oauth2_client_info?.authorization_request_uri || | ||
| defaultValue?.authorization_request_uri || | ||
| '', | ||
| token_request_uri: | ||
| encryptedExtra.oauth2_client_info?.token_request_uri || | ||
| defaultValue?.token_request_uri || | ||
| '', | ||
| scope: | ||
| encryptedExtra.oauth2_client_info?.scope || defaultValue?.scope || '', | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Suggestion: masked_encrypted_extra is parsed and then dereferenced without validating the parsed shape. If the backend returns malformed JSON (throws) or the valid JSON literal null, this component crashes during render (Cannot read properties of null). Parse defensively and default to an empty object when parsing fails or when the parsed value is not a plain object. [null pointer]
Severity Level: Major ⚠️
- ❌ Editing OAuth2-enabled database can crash connection modal.
- ⚠️ Prevents updating Google Sheets OAuth2 client settings.
- ⚠️ Unhandled JSON parse errors degrade admin UX.Steps of Reproduction ✅
1. Open the database connection modal, which renders `DatabaseModal` from
`superset-frontend/src/features/databases/DatabaseModal/index.tsx:70-82`; this uses
`DatabaseConnectionForm` (import at line 12) to render the dynamic form for a database.
2. In `DatabaseConnectionForm`
(`superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx:87-137`),
note that fields are rendered from `FORM_FIELD_MAP`, and
`FORM_FIELD_MAP.oauth2_client_info = OAuth2ClientField` in `constants.ts:72-80`, so any
database engine whose parameters schema exposes `oauth2_client_info` will render
`OAuth2ClientField`.
3. Construct or load a `DatabaseObject` (type defined in
`superset-frontend/src/features/databases/types.ts:67-124`) whose `masked_encrypted_extra`
property is the JSON literal string `"null"` (for example, by adding a new Jest test in
`OAuth2ClientField.test.tsx` that sets `defaultProps.db.masked_encrypted_extra = 'null'`
before calling `render(<OAuth2ClientField {...defaultProps} />)` as in lines 74-79).
4. When React renders `OAuth2ClientField` (`OAuth2ClientField.tsx:45-50`), the state
initializer `useState(deriveOauth2ClientInfo)` invokes `deriveOauth2ClientInfo` (line 51),
which executes `JSON.parse(db?.masked_encrypted_extra || '{}')` (line 52); for `"null"`
this returns `null`, and the following line `encryptedExtra.oauth2_client_info?.id` (line
54) attempts to access a property on `null`, causing a runtime error (`Cannot read
properties of null (reading 'oauth2_client_info')`) and crashing the form render.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx
**Line:** 51:67
**Comment:**
*Null Pointer: `masked_encrypted_extra` is parsed and then dereferenced without validating the parsed shape. If the backend returns malformed JSON (throws) or the valid JSON literal `null`, this component crashes during render (`Cannot read properties of null`). Parse defensively and default to an empty object when parsing fails or when the parsed value is not a plain object.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
The PR comment file contains only the header row and no actual comments. There is no content to analyze or act upon. |
Comment on lines
+30
to
+41
| const computeInitialIsPublic = ( | ||
| database: Partial<DatabaseObject> | null | undefined, | ||
| ): boolean => { | ||
| if (!database || database.engine !== Engines.GSheet) return true; | ||
| if ( | ||
| database.masked_encrypted_extra && | ||
| database.masked_encrypted_extra !== '{}' | ||
| ) { | ||
| return false; | ||
| } | ||
| if (database.parameters?.service_account_info) return false; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Suggestion: The initial public/private derivation does not check for oauth2_client_info in database.parameters, so an OAuth2-configured Google Sheets connection can be misclassified as public when masked_encrypted_extra is empty. That hides OAuth2 fields on edit and risks submitting an update that drops existing private-auth configuration. Include an explicit database.parameters?.oauth2_client_info check in the initial-state computation. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ GSheets OAuth2 databases can appear as public-only in UI.
- ⚠️ OAuth2 client controls hidden despite active OAuth2 configuration.
- ⚠️ Users may unknowingly misconfigure private sheet access.Steps of Reproduction ✅
1. Create or configure a Google Sheets database whose backend properties include OAuth2
client configuration only under `parameters.oauth2_client_info` and not under
`masked_encrypted_extra`, as exercised in
`test_validate_parameters_skips_oauth2_connections_with_parameters` at
`tests/unit_tests/db_engine_specs/test_gsheets.py:29-38` (properties.parameters contains
`"oauth2_client_info": {"id": "client-id", "secret": "client-secret"}` with no
`"masked_encrypted_extra"` key).
2. Open the edit modal for this database in the UI, which uses `DatabaseModal` to fetch
the resource and store it in React state via the `dbReducer` Fetched branch at
`superset-frontend/src/features/databases/DatabaseModal/index.tsx:149-195`, then renders
`<DatabaseConnectionForm db={db as DatabaseObject} ... />` in
`renderDatabaseConnectionForm` at
`superset-frontend/src/features/databases/DatabaseModal/index.tsx:88-96`.
3. On first render with the fetched `db`, `DatabaseConnectionForm` initializes and then
re-derives the `isPublic` state using `computeInitialIsPublic(db)` inside the `useEffect`
at
`superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx:74-85`;
since the current implementation only checks `database.masked_encrypted_extra` and
`database.parameters?.service_account_info` (lines 33-40) and does not inspect
`database.parameters?.oauth2_client_info`, it returns `true` even though OAuth2 client
info is present in parameters.
4. With `isPublic` incorrectly set to `true` for a GSheets engine, all private-auth UI is
hidden: `OAuth2ClientField` returns `null` when `db?.engine === Engines.GSheet &&
isPublic` at
`superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:80-82`,
`EncryptedField` hides the credentials section for GSheets when `isPublic` is true at
`superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:62-65`,
and `TableCatalog` hides the helper text about needing credentials at
`superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx:36-41`,
so editing this connection in the UI misclassifies it as "public-only" and prevents users
from seeing or managing their existing OAuth2 configuration.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx
**Line:** 30:41
**Comment:**
*Api Mismatch: The initial public/private derivation does not check for `oauth2_client_info` in `database.parameters`, so an OAuth2-configured Google Sheets connection can be misclassified as public when `masked_encrypted_extra` is empty. That hides OAuth2 fields on edit and risks submitting an update that drops existing private-auth configuration. Include an explicit `database.parameters?.oauth2_client_info` check in the initial-state computation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
849
to
855
| // CONNECTION_MISSING_PARAMETERS_ERROR is intentionally excluded | ||
| // here so required-field errors only surface on submit | ||
| // (onCreate=true), not on blur of empty inputs. | ||
| const allowed = [ | ||
| 'CONNECTION_MISSING_PARAMETERS_ERROR', | ||
| 'CONNECTION_ACCESS_DENIED_ERROR', | ||
| 'INVALID_PAYLOAD_SCHEMA_ERROR', | ||
| ]; |
Contributor
There was a problem hiding this comment.
Suggestion: Removing CONNECTION_MISSING_PARAMETERS_ERROR from the blur allow-list makes blur validation return an empty object for missing required fields, and the save flow currently relies on pre-populated validation state as a fallback because it checks errors?.length (array-style) on submit. This combination can let invalid dynamic-form database configs proceed to save with missing required parameters. Keep required-field errors represented in a way the submit path can reliably block (for example, ensure submit receives/uses object-key errors, or keep required-field state available for the save guard). [logic error]
Severity Level: Major ⚠️
- ❌ Dynamic-form database submit ignores frontend required-field gating.
- ⚠️ Users see failures only after backend rejects save.Steps of Reproduction ✅
1. Open the dynamic-form database connection UI so that `DatabaseModal` is rendered and
uses `useDatabaseValidation()` from `superset-frontend/src/views/CRUD/hooks.ts:37-115`,
wiring `[validationErrors, getValidation, ...]` into `DatabaseConnectionForm` at
`DatabaseModal/index.tsx:621-630` and then into `TableCatalog` via the `getValidation`
prop at `DatabaseModal/index.tsx:1893-1897`.
2. In the Google Sheets catalog UI, add a sheet row and blur the URL field without
entering a value; `TableCatalog` uses `ValidatedInput` with `validationMethods={{ onBlur:
getValidation }}` at `TableCatalog.tsx:7-11`, which calls `getValidation(db, false)`
(onCreate=false) from `useDatabaseValidation`.
3. The backend responds with a `CONNECTION_MISSING_PARAMETERS_ERROR` for the missing
required URL/host; in `useDatabaseValidation`, the `.filter` at `hooks.ts:69-78` excludes
this error type on blur because it only allows `CONNECTION_ACCESS_DENIED_ERROR` and
`INVALID_PAYLOAD_SCHEMA_ERROR` (see the comment block at `hooks.ts:69-75` and tests at
`hooks.test.tsx:22-39`), so `parsedErrors` reduces to `{}` and `validationErrors` state
becomes `{}` even though required fields are missing.
4. Because `hasValidated` is set `true` and `validationErrors` is `{}`, the "Connect"
button in `DatabaseModal` becomes enabled (`disabled` depends on `!hasValidated ||
isValidating || (validationErrors && Object.keys(validationErrors).length > 0)` at
`DatabaseModal/index.tsx:1290-23`). Clicking "Connect" calls `onSave` at
`DatabaseModal/index.tsx:869-69`, which re-runs `getValidation(dbToUpdate, true)`
(onCreate=true) and receives an object of required-field errors; however, the submit guard
checks `if (!isEmpty(validationErrors) || errors?.length)` at
`DatabaseModal/index.tsx:62-69`: `validationErrors` is still the previous `{}` from blur
(React state update from this submit call is not visible within the same render), and
`errors` is an object without a `length` property, so `errors?.length` is `undefined`. The
condition is false and `onSave` proceeds to call `createResource`/`updateResource` with
missing required parameters, relying solely on backend enforcement, which can let the
frontend bypass its intended submit-time gating for dynamic-form database configs.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/views/CRUD/hooks.ts
**Line:** 849:855
**Comment:**
*Logic Error: Removing `CONNECTION_MISSING_PARAMETERS_ERROR` from the blur allow-list makes blur validation return an empty object for missing required fields, and the save flow currently relies on pre-populated validation state as a fallback because it checks `errors?.length` (array-style) on submit. This combination can let invalid dynamic-form database configs proceed to save with missing required parameters. Keep required-field errors represented in a way the submit path can reliably block (for example, ensure submit receives/uses object-key errors, or keep required-field state available for the save guard).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
kgabryje
changed the title
…required errors Restores the "Type of Google Sheets allowed" dropdown that was removed in commit e2a9e0c6bb (PR apache#32048). Users selecting "Publicly shared sheets only" no longer see the service-account upload, OAuth2 client field, or the credentials helper text in the table catalog — those are only relevant for private sheets and were a source of confusion when they appeared by default. Also fixes an over-eager validation flash on the catalog Sheet Name and URL inputs: required-field errors used to fire on the first blur of an empty row, before the user had typed anything. Now they only surface on submit. This is implemented by dropping CONNECTION_MISSING_PARAMETERS_ERROR from the on-blur allow-list in useDatabaseValidation; other live-on-blur checks (invalid payload schema, access denied) are unchanged. The isPublic state is UI-only: no schema, migration, or API changes. State lives in DatabaseConnectionForm/index.tsx and is threaded to EncryptedField, TableCatalog, and OAuth2ClientField via the existing field-props bag. Edit mode defaults to private when an existing connection has masked_encrypted_extra or service_account_info set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nc OAuth2 field Addresses review feedback on the public/private dropdown: 1. Stale credentials were retained on private → public toggle. The save-time merge in `DatabaseModal/index.tsx` only writes a parameter into `masked_encrypted_extra` when truthy, so clearing `parameters.service_account_info` to '' was a no-op against credentials already loaded from `masked_encrypted_extra`. Fix: `handlePublicToggle` now also dispatches `onEncryptedExtraInputChange` with empty values, and the reducer's `EncryptedExtraInputChange` case deletes the key on empty/null instead of writing an empty value. 2. `OAuth2ClientField` initialized its local state from `db.masked_encrypted_extra` once at mount and never re-synced. After a public toggle cleared the underlying credentials, toggling back to private would show the original (now nonexistent) values. Fix: a `useEffect` re-derives local state whenever `db?.masked_encrypted_extra` changes. 3. `computeInitialIsPublic` reads `db?.masked_encrypted_extra`, but the `useEffect` that re-derives `isPublic` only depended on `db?.id` and `db?.engine`. Added `db?.masked_encrypted_extra` to the dep array so async edit-mode loads correctly default to "private" when the connection has stored credentials. 4. Replaced hardcoded `'gsheets'` string literals with the existing `Engines.GSheet` enum across the three consumers, matching the convention already used in `DatabaseModal/index.tsx`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DatabaseConnectionForm was calling each entry of FORM_FIELD_MAP as a
plain function rather than rendering it as a React element. Hooks
inside those components therefore registered against the parent's
fiber instead of their own. The form previously had no state of its
own, so the issue was latent — but adding `useState`/`useEffect` to
DatabaseConnectionForm for the gsheets public/private selector started
producing a "Rendered more hooks than during the previous render"
crash as soon as an engine was picked: the parent had 2 hooks on the
empty-engine render, then 2 + the picked engine's fields' hooks once
fields started rendering, breaking React's rule of hooks.
Switching to `<FieldComponent {...props} />` isolates each field's
hooks to its own fiber and makes the parent's hook count independent
of which fields are rendered. Verified against the dev env: opening
an existing Google Sheets connection and toggling
public→private→public→private no longer crashes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ng, OAuth2 layout
Four polish items bundled together:
1. Add vertical breathing room above the EncryptedField credential
section by giving `CredentialInfoForm` a `margin-top` of `sizeUnit *
4`. Previously the TableCatalog helper text sat directly against the
"Type of Google Sheets allowed" label with no gap.
2. Fix the always-160px-tall dropdown popup in the database modal. The
`.ant-select-dropdown` rule in `antDModalStyles` used `height`
instead of `max-height`, forcing the popup to a fixed size
regardless of content. With two short options the panel rendered
with ~100px of empty space below. Switching to `max-height`
preserves the cap for selects with many options while letting the
panel size to its content otherwise.
3. Stack the OAuth2 client information FormItem labels vertically
(label above input) by wrapping the FormItems in
`<Form layout="vertical">` inside the Collapse panel. Scoped to
OAuth2 only — no change to the surrounding Form layout.
4. Pass `style={{ width: '100%' }}` instead of `css` to the gsheets
and credential-method Selects, matching the surrounding convention
and the original PR apache#32048 reference.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s and revert validation tweak
- Add `.ant-select { margin: sizeUnit 0 sizeUnit*2 }` to `CredentialInfoForm` so the bare `<FormLabel> + <Select>` pairs in EncryptedField line up with the surrounding `LabeledErrorBoundInput` inputs (Display name, Service Account). Previously the gap below "Type of Google Sheets allowed" and "How do you want to enter service account credentials?" was visibly tighter than the rest of the modal.
- Revert the `CONNECTION_MISSING_PARAMETERS_ERROR` change in `useDatabaseValidation`. The on-blur required-field UX work is being deferred. Restored the original allow-list and removed the corresponding tests in `hooks.test.tsx` and the soft assertion in `TableCatalog.test.tsx`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t types
The Select from `@superset-ui/core/components` doesn't expose a top-level
`style` prop in its public type — only `css`. Switching back to the
object form `css={{ width: '100%' }}` keeps the visual fix from the
previous polish round and satisfies tsc. Verified locally with the full
`npm run type` (mirroring the CI lint-frontend tsc step).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Auth2-only edit mode
Two review-comment fixes:
1. `deriveOauth2ClientInfo` in OAuth2ClientField was vulnerable to
`db.masked_encrypted_extra` being the literal string `"null"`,
malformed JSON, or a JSON non-object value — `JSON.parse('null')`
returns null and the subsequent `.oauth2_client_info?.id` would
throw. Wrapped the parse in try/catch and added an object-type
guard before reading any properties. Effect is wider than master
because the new useEffect re-runs the helper on every
`masked_encrypted_extra` change.
2. `computeInitialIsPublic` now also returns false when
`parameters.oauth2_client_info` is present. An OAuth2-only gsheets
connection (no `masked_encrypted_extra`, no
`service_account_info`) previously defaulted to public in edit
mode, hiding the OAuth2 panel and risking a stale-save. Tests for
the function are colocated in a new `DatabaseConnectionForm.test.tsx`.
Verified with `npm run type` (full project), prettier, custom rules,
and the affected Jest suites (84 component tests pass).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tChange Mirrors the defensive pattern just added to `OAuth2ClientField.deriveOauth2ClientInfo`. The dbReducer case for `EncryptedExtraInputChange` (introduced when the gsheets dropdown toggle began clearing stored credentials) called `JSON.parse(masked_encrypted_extra)` and then mutated the result — which crashes when the stored value is the literal string "null", malformed JSON, or any non-object value. Wrapped the parse in try/catch and required the result to be a non-null, non-array object before mutating. Added `test.each` cases in `index.test.tsx` to lock in the recovery behavior for "null", malformed JSON, a JSON primitive, and a JSON array. Verified locally with the full `npm run type` and the `dbReducer` + DatabaseConnectionForm Jest suites (37 + 84 pass). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kgabryje
force-pushed
the
gsheets-public-private-selector
branch
from
493cffb to
15b653f
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #04e6ea
Actionable Suggestions - 1
-
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx - 1
- ESLint rule definition not found for react-hooks · Line 88-88
Additional Suggestions - 1
-
superset-frontend/src/features/databases/DatabaseModal/index.tsx - 1
-
Specific exception type in catch · Line 299-300Replace the bare `catch {}` with an explicit `SyntaxError` check per BITO.md rule 10033 (Replace Broad Exception Catches with Specific Exception Types). Currently any thrown object — not just `SyntaxError` — would be silently caught, masking unexpected errors from `JSON.parse`.
Code suggestion
--- a/superset-frontend/src/features/databases/DatabaseModal/index.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.tsx @@ -293,7 +293,11 @@ try { parsedUnknown = JSON.parse(trimmedState.masked_encrypted_extra || '{}'); - } catch { + } catch (e: unknown) { + if (e instanceof SyntaxError) { + parsedUnknown = {}; + } else { + throw e; + } + } - parsedUnknown = {}; - } const parsed: Record<string, unknown> =
-
Review Details
-
Files reviewed - 12 · Commit Range:
849352e..15b653f- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DatabaseConnectionForm.test.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.test.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.test.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx
- superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx
- superset-frontend/src/features/databases/DatabaseModal/index.test.tsx
- superset-frontend/src/features/databases/DatabaseModal/index.tsx
- superset-frontend/src/features/databases/DatabaseModal/styles.ts
- superset-frontend/src/features/databases/types.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| // gsheets dropdown toggles back to private after we cleared stored creds). | ||
| useEffect(() => { | ||
| setOauth2ClientInfo(deriveOauth2ClientInfo()); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- depend only on the serialized DB-side credentials |
Contributor
There was a problem hiding this comment.
ESLint rule definition not found for react-hooks
The react-hooks/exhaustive-deps ESLint rule is not recognized. Ensure the eslint-plugin-react-hooks package is installed and properly configured in your ESLint setup. This rule is essential for catching dependency array issues in React hooks.
Code Review Run #04e6ea
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
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
Restores the Type of Google Sheets allowed dropdown on the Google Sheets database-connection form. The dropdown was originally introduced to let users opt out of service-account / OAuth2 credential UI when they only intend to query publicly shared sheets, and was removed in commit
e2a9e0c6bb(PR #32048) during the OAuth2 rework. User feedback since then has flagged the missing toggle: the credential upload and helper text show by default even for public-only setups, which is confusing.This PR brings the dropdown back with two options:
When "public" is selected, the service-account credential input, the OAuth2 client information panel, and the credentials helper text in the table catalog are all hidden. They reappear when "private" is selected.
Edit-mode default: the dropdown shows "private" when the connection has any of
masked_encrypted_extra !== '{}',parameters.service_account_info, or stored OAuth2 client info — otherwise it defaults to "public".Toggling private → public clears in-flight
parameters.service_account_infoandparameters.oauth2_client_infoand also removes those keys frommasked_encrypted_extraso previously stored credentials aren't carried over after a toggle in edit mode. The reducer'sEncryptedExtraInputChangeaction now treats empty/null values as a delete to support this cleanly.isPublicis UI-only — no schema change, migration, or API change. State lives inDatabaseConnectionForm/index.tsxand is threaded toEncryptedField,TableCatalog, andOAuth2ClientFieldvia the existing field-props bag.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
service_account_info/oauth2_client_info.Plus Jest unit tests:
New / updated tests cover:
service_account_infoandoauth2_client_infofrom bothparametersandmasked_encrypted_extraOAuth2ClientFieldreturns null when gsheets + public, and re-syncs local state whenmasked_encrypted_extrais clearedTableCataloghides credentials helper text when gsheets + publicdbReducerdeletes themasked_encrypted_extrakey when value is emptyADDITIONAL INFORMATION