-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix (temp): Wrap Integrations beforeLoad in Try/Catch and RenderBannerGuard if order #3115
Fix (temp): Wrap Integrations beforeLoad in Try/Catch and RenderBannerGuard if order #3115
Conversation
WalkthroughThe changes update the permission handling and error management logic across multiple components. In the permission component, the conditional check for displaying the guard banner is moved to the top of the logic flow, removing redundancy. In the secret manager pages, the permission prop for the Secret Syncs tab has been updated to use a specialized permission action and the pass-through prop removed. The route file now uses type annotations and try-catch blocks to capture data fetching errors, redirecting as needed. Changes
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/permissions/ProjectPermissionCan.tsx (1)
60-62
: LGTM! Improved conditional flow.Moving the guard banner check to the top of the logic flow improves code clarity and efficiency by prioritizing access restriction handling.
Consider adding a comment explaining the order-dependent nature of these conditions for future maintainers:
+ // Check access restriction first to prioritize guard banner display if (!isAllowed && renderGuardBanner) { return <ProjectPermissionGuardBanner />; }
frontend/src/pages/secret-manager/IntegrationsListPage/route.tsx (1)
55-69
: Consider reducing code duplication in redirect logic.The redirect logic is duplicated across multiple catch blocks. Consider extracting it into a helper function.
+const redirectToIntegrations = (projectId: string, selectedTab: IntegrationsListPageTabs) => { + throw redirect({ + to: "/secret-manager/$projectId/integrations", + params: { projectId }, + search: { selectedTab } + }); +}; let integrations: TIntegration[]; try { integrations = await context.queryClient.ensureQueryData({ queryKey: workspaceKeys.getWorkspaceIntegrations(projectId), queryFn: () => fetchWorkspaceIntegrations(projectId) }); } catch { - throw redirect({ - to: "/secret-manager/$projectId/integrations", - params: { - projectId - }, - search: { selectedTab: IntegrationsListPageTabs.SecretSyncs } - }); + redirectToIntegrations(projectId, IntegrationsListPageTabs.SecretSyncs); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/permissions/ProjectPermissionCan.tsx
(1 hunks)frontend/src/pages/secret-manager/IntegrationsListPage/IntegrationsListPage.tsx
(2 hunks)frontend/src/pages/secret-manager/IntegrationsListPage/route.tsx
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check Frontend Type and Lint check
🔇 Additional comments (3)
frontend/src/pages/secret-manager/IntegrationsListPage/route.tsx (2)
6-11
: LGTM! Improved type safety.Added proper type imports for better type checking and code maintainability.
28-43
: LGTM! Enhanced error handling for secret syncs.Added proper try/catch block to handle permission failures gracefully with redirects.
frontend/src/pages/secret-manager/IntegrationsListPage/IntegrationsListPage.tsx (1)
102-108
: LGTM! Improved permission handling for secret syncs.Updated to use the specialized
ProjectPermissionSecretSyncActions.Read
permission for better access control granularity.
Description 📣
This PR wraps the beforeLoad on the integrations list page in a try/catch to handle permission failure (temporary). Also adjusts if statements in permission banner guard to properly display
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets
Summary by CodeRabbit
Refactor
New Features
Bug Fixes