Update Organizations and Groups module to use info banners#3563
Conversation
…to Update-Organizations-Groups-Module-to-use-InfoBanners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughOrganizationPicker now uses useBannerWithDelay for error/success banners, closes the modal and clears forbidden state before async cleanup, moves the Redux switching flag dispatch to just before navigation, conditionally refreshes or pushes /home, and documents the change in the changelog. ChangesOrganization Switching Notification Flow
Sequence DiagramsequenceDiagram
participant OrganizationPicker
participant ReactQuery
participant ReduxStore
participant Router
participant useBannerWithDelay
OrganizationPicker->>ReduxStore: clear forbidden state, close modal
OrganizationPicker->>ReactQuery: cancel/remove previous context keys
OrganizationPicker->>ReduxStore: setActiveGroup, setUserContext
OrganizationPicker->>ReduxStore: setOrganizationSwitching(isSwitching:true, switchingTo)
OrganizationPicker->>Router: if pathname==/home -> refresh() else push('/home')
Router->>ReduxStore: pathname-change effect clears organizationSwitching
OrganizationPicker->>useBannerWithDelay: showBannerWithDelay(success|error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/vertex/components/features/org-picker/organization-picker.tsx (1)
65-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix unreachable error banner in handleOrganizationChange (async failures won’t hit catch)
Insrc/vertex/components/features/org-picker/organization-picker.tsx(handleOrganizationChange, try/catch around lines 65-111), thetryblock contains noawait; it onlyvoidsqueryClient.cancelQueries(...), does synchronousremoveQueries/dispatch/routercalls, then shows the success banner. As a result, thecatchcan only handle synchronous throws, so the “Failed to switch organization” banner won’t surface for real async switch/query failures.♻️ Option: await the cancellation work so failures can reach the catch
- orgScopedQueryKeys.forEach((queryKey) => { - void queryClient.cancelQueries({ queryKey }) - queryClient.removeQueries({ queryKey }); - }); + await Promise.all( + orgScopedQueryKeys.map((queryKey) => + queryClient.cancelQueries({ queryKey }) + ) + ); + orgScopedQueryKeys.forEach((queryKey) => { + queryClient.removeQueries({ queryKey }); + });🤖 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/vertex/components/features/org-picker/organization-picker.tsx` around lines 65 - 111, The try/catch is currently ineffective for async failures because cancelQueries is voided and router calls are not awaited; update handleOrganizationChange to await the async operations so errors propagate to the catch: change the orgScopedQueryKeys loop to collect and await Promise.all(orgScopedQueryKeys.map(k => queryClient.cancelQueries({ queryKey: k }))) (keep queryClient.removeQueries synchronous afterwards), await router.push("/home") or await router.refresh() where used, and optionally accept the error parameter in the catch to include error details when calling showBannerWithDelay for the failure path; reference functions/identifiers: handleOrganizationChange, queryClient.cancelQueries, queryClient.removeQueries, router.push, router.refresh, showBannerWithDelay, setOrganizationSwitching.
🤖 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.
Outside diff comments:
In `@src/vertex/components/features/org-picker/organization-picker.tsx`:
- Around line 65-111: The try/catch is currently ineffective for async failures
because cancelQueries is voided and router calls are not awaited; update
handleOrganizationChange to await the async operations so errors propagate to
the catch: change the orgScopedQueryKeys loop to collect and await
Promise.all(orgScopedQueryKeys.map(k => queryClient.cancelQueries({ queryKey: k
}))) (keep queryClient.removeQueries synchronous afterwards), await
router.push("/home") or await router.refresh() where used, and optionally accept
the error parameter in the catch to include error details when calling
showBannerWithDelay for the failure path; reference functions/identifiers:
handleOrganizationChange, queryClient.cancelQueries, queryClient.removeQueries,
router.push, router.refresh, showBannerWithDelay, setOrganizationSwitching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5965c88-927a-4f48-a8cc-7f63cfec6942
📒 Files selected for processing (1)
src/vertex/components/features/org-picker/organization-picker.tsx
| message: `Switched to ${formatTitle(group.grp_title)}`, | ||
| scoped: false, | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
Please remove this success banner for Switched to ..., it is not necessary @BwanikaRobert
There was a problem hiding this comment.
@Codebmk Actually in the production even the error banner won't be shown because all the switching happens at the client side.
There was a problem hiding this comment.
The success banner has been removed. We only have the error banner left.
|
New azure vertex changes available for preview here |
|
One last change, please update changelog accordingly @BwanikaRobert |
|
New azure vertex changes available for preview here |
Codebmk
left a comment
Codebmk
left a comment
There was a problem hiding this comment.
Amazing thanks @BwanikaRobert
3 participants
Summary of Changes (What does this PR do?)
Replaces the ReusableToast call in OrganizationPicker with the centralized InfoBanner system.
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Closes [Vertex: Migration] Update Organizations & Groups Module to use InfoBanners #3452
Files handled as part of this issue (remaining items resolved in PR Vertex:(Migration)Update Network Management Module to use InfoBanners #3558):
src/vertex/app/(authenticated)/admin/networks/page.tsxsrc/vertex/app/(authenticated)/admin/networks/[id]/page.tsxsrc/vertex/app/(authenticated)/admin/networks/requests/page.tsxScreenshots
Summary by CodeRabbit
Improvements
Documentation