Fix: Bugs 10 Feb #535
Fix: Bugs 10 Feb #535
Conversation
WalkthroughThe pull request refactors how cloud account instances are detected by introducing a new function, Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/InstanceFilters/InstanceFilters.tsx (1)
67-67: Remove ts-ignore comment by fixing the type issue.The ts-ignore comment should be addressed by properly typing the
filterStateobject.Apply this diff to fix the type issue:
- const filterState = {}; + const filterState: Partial<InstanceFilterStatus> = {}; - //@ts-ignore return filterState as InstanceFilterStatus;
🧹 Nitpick comments (7)
app/(public)/(center-content)/change-password/page.tsx (1)
61-65: Remove ts-ignore by adding proper types.The type assertion
as anycan be replaced with proper types for better type safety.- changePasswordMutation.mutate({ - email: decodeURIComponent(email as string), - token: decodeURIComponent(token as string), - password: values.password, - } as any); + interface ChangePasswordPayload { + email: string; + token: string; + password: string; + } + + const payload: ChangePasswordPayload = { + email: decodeURIComponent(email ?? ''), + token: decodeURIComponent(token ?? ''), + password: values.password, + }; + + changePasswordMutation.mutate(payload);app/(public)/(center-content)/reset-password/components/ResetPasswordPage.tsx (1)
156-166: Remove ts-ignore by adding proper types.The ts-ignore comments can be replaced with proper types for better type safety.
- {/* @ts-ignore */} - <TextField + <TextField<string> name="email" id="email" placeholder="Input your registered email" value={values.email} onChange={handleChange} onBlur={handleBlur} error={touched.email && errors.email} helperText={touched.email && errors.email} /> {isReCaptchaSetup && ( - // @ts-ignore - <ReCAPTCHA + <ReCAPTCHA<string>Also applies to: 178-179
src/utils/access/byoaResource.js (1)
1-6: Consolidate similar functions and add documentation.The functions
checkIfResouceIsBYOAandisCloudAccountInstancehave similar logic. Consider consolidating them into a single utility function to reduce code duplication.+/** + * Checks if the provided ID contains the cloud account identifier. + * @param {string} id - The ID to check. + * @returns {boolean} - True if the ID contains the cloud account identifier. + */ export function checkIfResouceIsBYOA(id) { if (!id) { return false; } return id?.includes("r-injectedaccountconfig"); } +/** + * Checks if the provided instance is a cloud account instance. + * @param {Object} instance - The instance to check. + * @returns {boolean} - True if the instance is a cloud account instance. + */ export function isCloudAccountInstance(instance) { return instance?.resourceID?.includes("r-injectedaccountconfig"); } +/** + * Generic function to check if a value contains the cloud account identifier. + * @param {string} value - The value to check. + * @returns {boolean} - True if the value contains the cloud account identifier. + */ +function containsCloudAccountIdentifier(value) { + if (!value) { + return false; + } + return value?.includes("r-injectedaccountconfig"); +} + +export function checkIfResouceIsBYOA(id) { + return containsCloudAccountIdentifier(id); +} + +export function isCloudAccountInstance(instance) { + return containsCloudAccountIdentifier(instance?.resourceID); +}Also applies to: 8-10
app/(dashboard)/cloud-accounts/page.tsx (2)
98-105: Add proper type definitions to remove TypeScript ignore comments.Consider adding proper type definitions for
instance.result_paramsto improve type safety and remove the need for@ts-ignorecomments.interface ResultParams { gcp_project_id?: string; aws_account_id?: string; cloud_provider?: string; account_configuration_method?: string; } interface ResourceInstance { // ... other properties result_params: ResultParams; }
1-504: Consider breaking down the large component.The file is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more focused components:
- Extract the table columns configuration into a separate file
- Move the mutation logic into custom hooks
- Create separate components for different sections of the page
app/(dashboard)/instances/components/InstanceForm.tsx (1)
1-614: Consider breaking down the large form component.The file is quite large and handles multiple responsibilities. Consider:
- Extracting form validation logic into a custom hook
- Moving form sections into separate components
- Creating a separate component for the preview card
Example structure:
// hooks/useInstanceForm.ts export function useInstanceForm(config) { // Form validation and submission logic } // components/InstanceForm/ // ├── StandardInformation.tsx // ├── NetworkConfiguration.tsx // ├── DeploymentConfiguration.tsx // ├── PreviewCard.tsx // └── index.tsx🧰 Tools
🪛 Biome (1.9.4)
[error] 161-161: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 190-190: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 252-252: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 253-253: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 254-254: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 275-275: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
app/(dashboard)/instances/page.tsx (1)
389-449: Consider optimizing multiple filter passes.The filtering logic is well-organized, but there are multiple separate filter passes over the instances array (
failedInstances,overloadedInstances,unhealthyInstances). Consider combining these into a single pass for better performance with large datasets.- const failedInstances = useMemo(() => { - return filteredInstances.filter((instance) => instance.status === "FAILED"); - }, [filteredInstances]); - - const overloadedInstances = useMemo(() => { - return filteredInstances.filter( - (instance) => instance.instanceLoadStatus === "POD_OVERLOAD" - ); - }, [filteredInstances]); - - const unhealthyInstances = useMemo(() => { - return filteredInstances.filter((instance) => { - const instanceHealthStatus = getInstanceHealthStatus( - instance.detailedNetworkTopology as Record< - string, - ResourceInstanceNetworkTopology - >, - instance.status as string - ); - if (instanceHealthStatus === "UNHEALTHY") return true; - return false; - }); - }, [filteredInstances]); + const { failedInstances, overloadedInstances, unhealthyInstances } = useMemo(() => { + return filteredInstances.reduce((acc, instance) => { + if (instance.status === "FAILED") { + acc.failedInstances.push(instance); + } + if (instance.instanceLoadStatus === "POD_OVERLOAD") { + acc.overloadedInstances.push(instance); + } + const instanceHealthStatus = getInstanceHealthStatus( + instance.detailedNetworkTopology as Record<string, ResourceInstanceNetworkTopology>, + instance.status as string + ); + if (instanceHealthStatus === "UNHEALTHY") { + acc.unhealthyInstances.push(instance); + } + return acc; + }, { + failedInstances: [], + overloadedInstances: [], + unhealthyInstances: [] + }); + }, [filteredInstances]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/(dashboard)/cloud-accounts/page.tsx(2 hunks)app/(dashboard)/dashboard/components/DashboardLogsTableHeader.tsx(1 hunks)app/(dashboard)/instances/[serviceId]/[servicePlanId]/[resourceId]/[instanceId]/[subscriptionId]/utils.ts(1 hunks)app/(dashboard)/instances/components/InstanceForm.tsx(2 hunks)app/(dashboard)/instances/components/InstancesTableHeader.tsx(2 hunks)app/(dashboard)/instances/page.tsx(2 hunks)app/(dashboard)/settings/page.tsx(0 hunks)app/(public)/(center-content)/change-password/page.tsx(3 hunks)app/(public)/(center-content)/layout.tsx(1 hunks)app/(public)/(center-content)/reset-password/components/ResetPasswordPage.tsx(3 hunks)src/components/InstanceFilters/InstanceFilters.tsx(3 hunks)src/components/ResourceInstance/AuditLogs/AuditLogs.tsx(2 hunks)src/utils/access/byoaResource.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/(dashboard)/settings/page.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/ResourceInstance/AuditLogs/AuditLogs.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: package
🔇 Additional comments (12)
app/(public)/(center-content)/layout.tsx (1)
5-5: LGTM!The simplified layout with removed logo-related logic improves modularity by delegating logo display responsibility to individual pages.
app/(public)/(center-content)/change-password/page.tsx (1)
74-84: LGTM!The logo display logic is well-implemented and consistent with other pages.
app/(public)/(center-content)/reset-password/components/ResetPasswordPage.tsx (1)
135-145: LGTM!The logo display logic is well-implemented and consistent with other pages.
app/(dashboard)/dashboard/components/DashboardLogsTableHeader.tsx (1)
1-23: LGTM! Clean and consistent changes.The changes improve UX by:
- Using consistent terminology ("audit logs" instead of "events")
- Opening the link in a new tab
- Adding a visual indicator for external link (arrow icon)
src/components/InstanceFilters/InstanceFilters.tsx (2)
20-20: LGTM! Good constraint for button width.The maxWidth constraint helps maintain consistent button sizes across the UI.
95-101: LGTM! Improved layout and alignment.The styling changes enhance the visual consistency by:
- Distributing button widths equally
- Centering content both horizontally and vertically
- Using consistent spacing
Also applies to: 129-133
app/(dashboard)/instances/[serviceId]/[servicePlanId]/[resourceId]/[instanceId]/[subscriptionId]/utils.ts (1)
27-27: LGTM! Verify terminology consistency across the codebase.The label change from "Activities" to "Events" aligns with the broader terminology update.
Let's verify the terminology consistency across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining instances of "Activities" that might need to be updated # and verify that "Events" is used consistently in the context of audit logs. echo "Checking for remaining instances of 'Activities' in the codebase:" rg -i "activities" -g '!*.{json,md,lock}' echo -e "\nVerifying consistent usage of 'Events' in audit logs context:" rg -i "events.*audit|audit.*events" -g '!*.{json,md,lock}'Length of output: 4743
LGTM – Audit log terminology update verified.
The change from "Activities" to "Events" in the audit logs tab aligns with the overall usage across audit-related files. The shell script results confirm that in the audit log context (e.g., in API endpoints, hooks, and schema files), the term "Events" (or related "audit-events") is used consistently. The remaining instances of "activities" appear in non–audit log contexts and are not affected by this bug fix.
app/(dashboard)/instances/components/InstancesTableHeader.tsx (1)
363-437: LGTM! Well-structured Select component implementation.The Select component implementation is well-structured with:
- Proper conditional rendering based on available actions and selected instance
- Clear user feedback through tooltips
- Consistent styling with the design system
- Type-safe implementation
Also applies to: 480-493
app/(dashboard)/cloud-accounts/page.tsx (1)
90-110: LGTM! Clean implementation of cloud account instances filtering.The use of
isCloudAccountInstanceutility function improves code readability.app/(dashboard)/instances/components/InstanceForm.tsx (1)
393-412: LGTM! Clean implementation of cloud account instances filtering.The use of
isCloudAccountInstanceutility function improves code readability.app/(dashboard)/instances/page.tsx (2)
514-668: LGTM! Well-structured UI implementation.The UI implementation is clean and follows best practices:
- Good separation of concerns with dedicated components for different features
- Proper handling of loading states
- Clear modal/dialog management
57-57: LGTM! Verify the utility function behavior.Good refactoring to extract the filtering logic into a dedicated utility function. This improves code maintainability and reusability.
Let's verify that the utility function handles all edge cases correctly:
Also applies to: 391-391
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all usages of the isCloudAccountInstance function rg "isCloudAccountInstance" -A 3 # Search for the implementation of isCloudAccountInstance ast-grep --pattern 'export function isCloudAccountInstance($_) { $$$ }'Length of output: 2404
LGTM – The utility function behaves as intended.
The isCloudAccountInstance function correctly checks for the "r-injectedaccountconfig" substring in the resourceID and is being used appropriately across files (e.g., in both InstanceForm.tsx and page.tsx). No further changes are necessary.
Signed-off-by: nihalmohammed18 <129756850+nihalmohammed18@users.noreply.github.com>
Summary by CodeRabbit
New Features
Style