UN-3398 [FIX] Pre-populate connector form with schema defaults#1913
Merged
muhammad-ali-e merged 7 commits intomainfrom Apr 15, 2026
Merged
UN-3398 [FIX] Pre-populate connector form with schema defaults#1913muhammad-ali-e merged 7 commits intomainfrom
muhammad-ali-e merged 7 commits intomainfrom
Conversation
Contributor
Summary by CodeRabbit
WalkthroughUpdated AddSource.jsx to compute and populate form data using Ajv8 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kirtimanmishrazipstack
changed the title
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/components/input-output/add-source/AddSource.jsx | Adds getDefaultFormState to pre-populate form data with schema defaults for new connectors; fixes the isLLMWPaidSchema stale-closure issue by adding it to the useEffect dependency array. The metadata prop is intentionally omitted from the first effect's deps (handled by the second useEffect). |
| unstract/connectors/src/unstract/connectors/filesystems/sharepoint/sharepoint.py | Error message in _get_onedrive_drive updated to better explain options (Site URL vs. User Email); backward-compatible change with no logic modifications. |
| unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json | Removes is_personal from the top-level schema (was incorrectly shown for all auth types); renames "Drive ID (Optional)" to "Drive ID", which drops the optional hint even though the field remains non-required. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useEffect fires
selectedSourceId or isLLMWPaidSchema changed] --> B{selectedSourceId set?}
B -- No --> C[Reset spec & oAuthProvider]
B -- Yes --> D[Fetch connector/adapter schema]
D --> E{isLLMWPaidSchema?}
E -- Yes --> F[transformLlmWhispererJsonSchema]
E -- No --> G[Use raw json_schema]
F --> H[setSpec jsonSchema]
G --> H
H --> I{metadata non-empty?}
I -- Yes
existing connector --> J[setFormData metadata]
I -- No
new connector --> K[getDefaultFormState
validator, jsonSchema, empty, jsonSchema]
K --> L[setFormData defaults]
J --> M[Set oAuthProvider if OAuth]
L --> M
Prompt To Fix All With AI
This is a comment left during a code review.
Path: unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json
Line: 22-26
Comment:
**"(Optional)" hint removed from Drive ID title**
The title was changed from `"Drive ID (Optional)"` to `"Drive ID"`, but the field has no `required` constraint and its description still reads "Leave empty to use the default drive." Users now lose the at-a-glance hint that the field is optional. Consider restoring the qualifier or adding a note like "(optional)" in the description's first sentence.
```suggestion
"drive_id": {
"type": "string",
"title": "Drive ID (Optional)",
"description": "Specific Drive/Document Library ID. Leave empty to use the default drive."
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: unstract/connectors/src/unstract/connectors/filesystems/sharepoint/sharepoint.py
Line: 172-182
Comment:
**Error title mismatches the triggering condition**
This branch is reached because `user_email` is absent (`not self.user_email`), yet the message opens with "No Site URL provided." A user who intentionally omitted `site_url` to reach OneDrive will be confused by the leading sentence — they know they didn't provide a site URL, that was deliberate. The rest of the message is helpful; only the opening phrase needs adjustment.
```suggestion
error_msg = (
"Missing credentials for OneDrive access with Client Credentials. "
"To access a SharePoint site, provide the Site URL "
"(e.g., https://contoso.sharepoint.com/sites/mysite). "
"To access OneDrive, provide the User Email "
"(e.g., user@company.onmicrosoft.com)."
)
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "Merge branch 'main' into fix/share-prod-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/input-output/add-source/AddSource.jsx`:
- Around line 126-128: The useEffect that initializes spec/form defaults is
missing dependencies causing stale schema selection; update the useEffect
dependency array to include isLLMWPaidSchema (or planType if that's what drives
it), selectedSourceId, metadata, isConnector, and any other variables used
inside the effect so it reruns when the schema source changes, and ensure the
branch that computes jsonSchema (using transformLlmWhispererJsonSchema and
data?.json_schema) executes when those dependencies change so spec and form
defaults are always initialized from the correct schema.
🪄 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: b61c06e7-dd8a-4e79-861a-d4c399f14ad4
📒 Files selected for processing (1)
frontend/src/components/input-output/add-source/AddSource.jsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json (1)
35-77:⚠️ Potential issue | 🟠 MajorSchema removal breaks personal account routing; failing test confirms the regression.
is_personalis required by backend code at lines 417 and 499 inunstract/connectors/src/unstract/connectors/filesystems/sharepoint/sharepoint.pyto route personal accounts to the "consumers" tenant (line 499–500). With this field removed from the JSON schema, users cannot setis_personal=true, causing all accounts to default to business tenant routing (line 417 defaults toFalse).Additionally,
unstract/connectors/tests/filesystems/test_sharepoint_fs.py:118–123contains a test (test_json_schema_has_is_personal) that explicitly assertsis_personalis in the schema—this test will fail.Either restore
is_personalto the schema under theclient_credentialsbranch (with appropriate title/description), or update the backend to infer personal accounts via an alternative mechanism (e.g.,tenant_id="consumers"detection) and remove the failing test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json` around lines 35 - 77, The JSON schema removal of is_personal breaks backend routing and tests—restore is_personal into the "client_credentials" branch of the dependencies so callers can set personal accounts; add an entry named "is_personal" with type "boolean", a clear title (e.g., "Personal Account"), a description that it forces routing to the consumers tenant when true, and do NOT add it to the required array (keep required as ["tenant_id","client_id","client_secret"]); ensure the property name matches the backend checks in sharepoint.py (is_personal) so existing logic at the is_personal checks and the test test_json_schema_has_is_personal continue to pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json`:
- Around line 35-77: The JSON schema removal of is_personal breaks backend
routing and tests—restore is_personal into the "client_credentials" branch of
the dependencies so callers can set personal accounts; add an entry named
"is_personal" with type "boolean", a clear title (e.g., "Personal Account"), a
description that it forces routing to the consumers tenant when true, and do NOT
add it to the required array (keep required as
["tenant_id","client_id","client_secret"]); ensure the property name matches the
backend checks in sharepoint.py (is_personal) so existing logic at the
is_personal checks and the test test_json_schema_has_is_personal continue to
pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c85b42fd-36bb-48a1-b8a0-c6c0d6694de2
📒 Files selected for processing (1)
unstract/connectors/src/unstract/connectors/filesystems/sharepoint/static/json_schema.json
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/input-output/add-source/AddSource.jsx (1)
103-157:⚠️ Potential issue | 🟠 MajorEffect dependencies are still incomplete (stale closure risk).
This effect uses
isConnector,getUrl,axiosPrivate,metadata,handleException,setAlertDetails, andsetOpen, but the dependency array only includesselectedSourceIdandisLLMWPaidSchema. It can still compute/fetch with stale values when those inputs change without a source-id change.Suggested dependency update
- }, [selectedSourceId, isLLMWPaidSchema]); + }, [ + selectedSourceId, + isLLMWPaidSchema, + isConnector, + getUrl, + axiosPrivate, + metadata, + handleException, + setAlertDetails, + setOpen, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/input-output/add-source/AddSource.jsx` around lines 103 - 157, The useEffect in AddSource.jsx has a stale-closure risk because it references isConnector, getUrl, axiosPrivate, metadata, handleException, setAlertDetails and setOpen but only lists selectedSourceId and isLLMWPaidSchema; update the effect's dependency array to include all referenced external values (isConnector, getUrl, axiosPrivate, metadata, handleException, setAlertDetails, setOpen, and any setter/state vars like setIsLoading, setSpec, setFormData, setOAuthProvider if they come from props/context) so the effect re-runs when those change, or alternatively wrap callback helpers (e.g., getUrl, axiosPrivate, handleException, setAlertDetails, setOpen) with useCallback/useRef at their definitions to stabilize them and remove from deps—locate the useEffect block in AddSource.jsx and modify the dependency array or stabilize referenced symbols accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/input-output/add-source/AddSource.jsx`:
- Around line 103-157: The useEffect in AddSource.jsx has a stale-closure risk
because it references isConnector, getUrl, axiosPrivate, metadata,
handleException, setAlertDetails and setOpen but only lists selectedSourceId and
isLLMWPaidSchema; update the effect's dependency array to include all referenced
external values (isConnector, getUrl, axiosPrivate, metadata, handleException,
setAlertDetails, setOpen, and any setter/state vars like setIsLoading, setSpec,
setFormData, setOAuthProvider if they come from props/context) so the effect
re-runs when those change, or alternatively wrap callback helpers (e.g., getUrl,
axiosPrivate, handleException, setAlertDetails, setOpen) with useCallback/useRef
at their definitions to stabilize them and remove from deps—locate the useEffect
block in AddSource.jsx and modify the dependency array or stabilize referenced
symbols accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cf9042d-f6b3-4b08-8b14-bcbaabd4d1d8
📒 Files selected for processing (2)
frontend/src/components/input-output/add-source/AddSource.jsxunstract/connectors/src/unstract/connectors/filesystems/sharepoint/sharepoint.py
kirtimanmishrazipstack
requested review from
harini-venkataraman and
muhammad-ali-e
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
harini-venkataraman
approved these changes
Apr 15, 2026
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
muhammad-ali-e
approved these changes
Apr 15, 2026
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
AddSource.jsxto populate form data with JSON schema default values when creating a new connectorgetDefaultFormStatefrom@rjsf/utilsto extract defaults from the connector's JSON schemais_personalfield from SharePoint top-level schema (was only relevant for Client Credentials)site_urlerror messageWhy
connectorName) were shown in the UI but not included in the form data. Submitting without manually touching these fields caused a validation error ("This field is required") despite a default value being visible.is_personalfield was incorrectly at the top level, making it visible for OAuth users who don't need it.How
getDefaultFormStateutility to initializeformDatawith schema defaults for new connectors (whenmetadatais empty). Existing connectors continue to use their saved metadata.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
is_personalfield removal has no backend impact — the connector code still handles the field if present in saved metadata.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
@rjsf/utilsand@rjsf/validator-ajv8already in package.json)Notes on Testing
is_personalat top levelScreenshots
Checklist
I have read and understood the Contribution Guidelines.