fix: restrict ingestion to containers section shows up correctly and error message isn't duplicated#2071
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Azure Blob settings dialog now controls container selector visibility from saved defaults, connection-test results, authentication changes, and submission errors. The form exposes visibility and authentication-change callbacks and conditionally renders the container selector. ChangesAzure Blob container visibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/enhancements/connectors/azure-blob/settings-dialog.tsx`:
- Around line 178-179: Update the onAuthModeChange handler in the settings
dialog to reset containers to null in addition to hiding the container selector,
while preserving selectedContainers. This ensures the refresh action and
onSubmit validation require a new container test after authentication changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cfc11e4-f80b-4fdb-a91b-e1e84afe990f
📒 Files selected for processing (2)
frontend/enhancements/connectors/azure-blob/settings-dialog.tsxfrontend/enhancements/connectors/azure-blob/settings-form.tsx
richardz403
left a comment
There was a problem hiding this comment.
LGTM
Ran PR locally, the fix does solve the issue of multiple error messages being displayed, removing the excess error messages. The application itself also runs smoothly with no other bugs found.
Ran tests locally, all tests passed, no apparent issues.
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 1
- See PR comments: (1a) (1b)
Functional Review 1
- ✅ Successfully verified scenarios outlined in issue's expected behavior
⚠️ Additional edge case scenarios identified by (1a) (1b)
…ners section" when state is true
Issue - #2047 Summary - Fix nit: Abort-on-unmount cleanup never fires on dialog close, so a canceled test can still land and repopulate stale containers
40bc811 to
189ed1a
Compare
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 2
- ✅ Approved / LGTM 🚀
- ℹ️ Found a minor nit for an extreme edge case
- Abort-on-unmount cleanup never fires on dialog close, so a canceled test can still land and repopulate stale containers
- Addressed with a very small change / commit (since it was trivial)
- ✅ Resolved conflict & rebased branch off of the latest from main
Functional Review 2
- ✅ Successfully verified all test scenarios
Summary
Fixes #2047 .This PR adds a showContainers state to control when the "Restrict Ingestion to Containers" section shows up and onAuthModeChange to reset it when the user switches authentication methods, preventing stale container data from persisting across auth mode changes. The section also now hides on a failed test connection, so bad credentials clear any previously visible list. Additionally, setContainersError(null) is called at the start of onSubmit to prevent duplicate error messages when a stale connection error and a save validation error would otherwise both appear simultaneously.
Before
screen-capture.11.webm
After
screen-capture.13.webm
Changes
settings-dialog.tsx: adds showContainers state, sets it on test success/failure and auth mode change, clears containersError on submitsettings-form.tsx: accepts showContainers + onAuthModeChange props, gates the container section on both, resets on tab switchSummary by CodeRabbit