fix: s3 button behaviours#1082
Conversation
Add env-var fallback getters for S3 credentials and clearer errors: implement get_client_id and get_client_secret to read from config or AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY and raise ValueError when missing. Frontend fixes: treat a connected connector as available in the connector card UI, and surface S3 bucket loading errors on the upload page by including the query error in the response and rendering a descriptive error message when bucket fetch fails.
Connector card: use isConnected alone for active state, treat a connector as configured when isConnected or connector.available, and show a "Configure" action (with Settings2 icon) when onConfigure is provided; also keep existing loading state. S3 settings dialog: import useEffect and add an effect to sync buckets and selectedBuckets when defaults.bucket_names load asynchronously so defaults are applied after dialog mount. AWS logo: replace fill="currentColor" with an explicit color (#232F3E) for consistent rendering.
github-actions
Bot
added
frontend
There was a problem hiding this comment.
Pull request overview
Improves the AWS S3 connector UX by refining connector state/action rendering in settings, making S3 bucket loading failures visible to users, and enhancing backend credential resolution behavior to support config/env fallback.
Changes:
- Refines connector card state handling and available actions for direct-config connectors (S3/COS) vs connected connectors.
- Adds UI synchronization/error handling for S3 settings + bucket status loading flows.
- Adds S3 connector
get_client_id/get_client_secretmethods to support config/env fallback and clearer missing-credential errors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/connectors/aws_s3/connector.py | Adds credential getter methods with config→env fallback and explicit error messages. |
| frontend/components/icons/aws-logo.tsx | Updates AWS logo fill color to AWS brand color. |
| frontend/app/upload/[provider]/page.tsx | Displays a user-facing error state when S3 bucket status loading fails. |
| frontend/app/settings/_components/s3-settings-dialog.tsx | Syncs bucket selection state when defaults load asynchronously after mount. |
| frontend/app/settings/_components/connector-card.tsx | Adjusts connector state display/actions to better reflect “connected” vs “available”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| def get_client_id(self) -> str: | ||
| """Return access key from config dict, or AWS_ACCESS_KEY_ID env var as fallback.""" | ||
| val = self.config.get("access_key") or os.getenv("AWS_ACCESS_KEY_ID") | ||
| if val: | ||
| return val | ||
| raise ValueError( | ||
| "S3 credentials not set. Provide 'access_key' in the connector config " | ||
| "or set the AWS_ACCESS_KEY_ID environment variable." | ||
| ) | ||
|
|
||
| def get_client_secret(self) -> str: | ||
| """Return secret key from config dict, or AWS_SECRET_ACCESS_KEY env var as fallback.""" | ||
| val = self.config.get("secret_key") or os.getenv("AWS_SECRET_ACCESS_KEY") | ||
| if val: |
There was a problem hiding this comment.
In get_client_id() / get_client_secret(), the env var names are duplicated as string literals ("AWS_ACCESS_KEY_ID" / "AWS_SECRET_ACCESS_KEY") even though the class already defines CLIENT_ID_ENV_VAR / CLIENT_SECRET_ENV_VAR. Using the constants here avoids drift if the env var names ever change and keeps the credential resolution logic consistent with the rest of the connector base classes.
| {isConnected || connector?.available | ||
| ? `${connector.name} is configured.` | ||
| : "Not configured."} |
There was a problem hiding this comment.
connector.available is derived from whether required env vars are present (see ConnectionManager._is_connector_available), not whether the user has actually created/authenticated a connection. Using isConnected || connector.available to display “is configured” can mislabel a connector as configured when it’s merely available (e.g., env vars set but no active connection). Consider basing the “configured” wording strictly on isConnected, and using separate copy for “available to configure” vs “not available” states.
| // Sync bucket state when defaults load asynchronously after dialog mount | ||
| useEffect(() => { | ||
| if (defaults?.bucket_names?.length) { | ||
| setBuckets(defaults.bucket_names); | ||
| setSelectedBuckets((prev) => | ||
| prev.length ? prev : defaults.bucket_names, | ||
| ); | ||
| } | ||
| }, [defaults?.bucket_names?.join(",")]); |
There was a problem hiding this comment.
The useEffect dependency on defaults?.bucket_names?.join(",") can produce collisions when bucket names contain commas (different arrays can stringify to the same value), and it makes the dependency semantics less clear. Prefer depending on defaults?.bucket_names directly (or a stable key like JSON.stringify(defaults.bucket_names)), and keep the deep-compare logic explicit if that’s what you want.
This pull request introduces several improvements to the AWS S3 connector experience, both in the frontend UI and backend connector logic. The main changes include better handling and display of connector states in the UI, improved error handling for S3 bucket loading, and enhanced credential fallback logic in the backend connector.
Frontend UI improvements:
ConnectorCard, ensuring that the UI more accurately reflects whether a connector is configured, available, or requires setup. [1] [2] [3] [4] [5]Backend connector improvements:
get_client_idandget_client_secretmethods to theS3Connectorclass, allowing credentials to be pulled from either the config dictionary or environment variables, and providing clear error messages when credentials are missing.