AWS Integration: Account settings modal #6808

ahmadshaheer · 2025-01-13T07:28:19Z

Summary

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas

Important

Add AWS account settings modal for configuration and region management, with UI enhancements and refactoring for integration management.

Features:
- Add AccountSettingsModal in AccountActions.tsx for AWS account configuration.
- Implement updateAccountConfig in index.ts to update AWS account settings.
- Add ServiceStatus component in ServiceDetails.tsx to display service connection status.
UI Enhancements:
- Update SignozModal to use rootClassName instead of className.
- Add styles for AccountSettingsModal and CloudAccountSetupModal.
- Add IntegrationsUninstallBar to AccountSettingsModal.tsx for uninstalling integrations.
Refactoring:
- Refactor useAccountSettingsModal and useIntegrationModal hooks for better state management.
- Rename ServiceStatus interface to IServiceStatus in types.ts.

^{This description was created by}^{for 0a362df. It will automatically update as commits are pushed.}

github-actions · 2025-01-13T07:28:33Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

github-actions · 2025-01-13T07:28:51Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

❌ Changes requested. Reviewed everything up to a976e18 in 1 minute and 54 seconds

More details

Looked at 826 lines of code in 14 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountSettingsModal.tsx:99

Draft comment:
Consider using native array methods for comparison instead of isEqual from lodash-es for better performance, especially if the arrays are simple.
Reason this comment was not posted:
Confidence changes required: 50%
The code uses isEqual from lodash-es which might not be necessary for simple array comparison. Using native methods could improve performance.

2. frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountSettingsModal.tsx:6

Draft comment:
Consider using native array methods for comparison instead of isEqual from lodash-es for better performance, especially if the arrays are simple. This comment also applies to other instances in this file.
Reason this comment was not posted:
Confidence changes required: 50%
The code uses isEqual from lodash-es which might not be necessary for simple array comparison. Using native methods could improve performance.

3. frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountActions.tsx:97

Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable on line 97.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_h9GQt5mdQGoTAJe1

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-01-13T07:30:25Z

frontend/src/api/integrations/aws/index.ts

+	payload: AccountConfigPayload,
+): Promise<AccountConfigResponse> => {
+	const response = await axios.put<AccountConfigResponse>(
+		`http://localhost:3000/api/v1/cloud-integrations/aws/accounts/${accountId}/config`,


Avoid using hardcoded URLs for API requests. Consider using environment variables to manage API endpoints. This comment also applies to other instances in this file.

frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountActions.tsx

ellipsis-dev · 2025-01-13T07:30:25Z

frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountSettingsModal.tsx

+									</button>
+								</div>
+								<Select
+									style={{ height: '44px' }}


Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead.

github-actions · 2025-01-14T10:11:00Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

❌ Changes requested. Incremental review on 294b1e1 in 1 minute and 3 seconds

More details

Looked at 1703 lines of code in 17 files
Skipped 0 files when reviewing.
Skipped posting 4 drafted comments based on config settings.

1. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountActions.tsx:143

Draft comment:
Consider adding a null check for activeAccount before passing it to AccountSettingsModal to avoid potential runtime errors.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The AccountSettingsModal button is only shown when accounts?.length is truthy (line 92). When this is true, activeAccount will always have a value because initialAccount is set to accounts[0] when accounts exist (line 64). The type assertion is actually unnecessary but not harmful. A null check isn't needed here because the component's logic prevents this case.
I could be wrong about the initialization logic - there might be edge cases where accounts array becomes empty after initialization. Also, the type assertion suggests the original author was unsure about nullability.
The component's structure ensures activeAccount will have a value when the modal can be opened - the button to open it only appears when accounts exist, and activeAccount is always initialized with accounts[0] in this case.
The comment should be deleted because the suggested null check is unnecessary given the component's logic that prevents this edge case.

2. frontend/src/components/SignozModal/SignozModal.tsx:8

Draft comment:
Consider handling both className and rootClassName props for better flexibility and to avoid confusion.
Reason this comment was not posted:
Confidence changes required: 50%
The SignozModal component in SignozModal.tsx uses rootClassName but the className prop is not used. This could lead to confusion or missed styling opportunities. It's better to handle both className and rootClassName for flexibility.

3. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:45

Draft comment:
Wrap renderRegionSelector in useCallback to optimize performance and prevent unnecessary re-renders.
Reason this comment was not posted:
Confidence changes required: 40%
The AccountSettingsModal.tsx file imports useCallback but does not use it for the renderRegionSelector function. This could lead to unnecessary re-renders. Wrapping renderRegionSelector in useCallback would optimize performance.

4. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:94

Draft comment:
Wrap renderAccountDetails in useCallback to optimize performance and prevent unnecessary re-renders.
Reason this comment was not posted:
Confidence changes required: 40%
The AccountSettingsModal.tsx file imports useCallback but does not use it for the renderAccountDetails function. This could lead to unnecessary re-renders. Wrapping renderAccountDetails in useCallback would optimize performance.

Workflow ID: wflow_ypwtuKbzg9EahAZt

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-01-14T10:11:44Z

frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx

+					</button>
+				</div>
+				<Select
+					style={{ height: '44px' }}


Avoid using inline styles in React components. Consider using CSS classes or styled components instead.

github-actions · 2025-01-14T12:51:18Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on 7305c2d in 52 seconds

More details

Looked at 33 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. frontend/src/pages/IntegrationsModulePage/IntegrationsModulePage.tsx:5

Draft comment:
Remove the old import statement for CloudIntegrationPage to avoid redundancy and confusion.
Reason this comment was not posted:
Comment looked like it was already resolved.

2. frontend/src/pages/IntegrationsModulePage/IntegrationsModulePage.tsx:5

Draft comment:
Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This applies to the CloudIntegrationPage import.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_OozjWcDG1mHTUnAR

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

… to the integrations modal

…vements

github-actions · 2025-01-14T13:07:26Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

❌ Changes requested. Incremental review on 0a362df in 1 minute and 13 seconds

More details

Looked at 1724 lines of code in 18 files
Skipped 0 files when reviewing.
Skipped posting 5 drafted comments based on config settings.

1. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountActions.tsx:143

Draft comment:
Consider adding a null check for activeAccount before rendering AccountSettingsModal to avoid potential runtime errors.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The AccountSettingsModal is only accessible via a button that's inside a conditional block (accounts?.length). This means the button to open the modal is only shown when there are accounts, and in that case activeAccount will always have a value (either from URL or first account). The force type casting is actually safe here because the UI flow prevents null cases.
I could be wrong about the UI flow - there might be edge cases where accounts data is loaded/changed after initial render. Also, defensive programming generally favors explicit null checks over implicit safety.
The component's logic ensures activeAccount is populated before the modal can be opened - the type assertion is a implementation detail that doesn't create actual runtime risk. Adding a null check would be defensive programming without practical benefit.
The comment should be deleted because the suggested null check is unnecessary - the component's structure already ensures type safety through its UI flow.

2. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:104

Draft comment:
Since account is a required prop, the optional chaining (?.) is unnecessary. Consider removing it for cleaner code. This applies to other instances in this file as well.
Reason this comment was not posted:
Confidence changes required: 50%
The AccountSettingsModal component uses account?.id in multiple places. Since account is a required prop, the optional chaining is unnecessary and can be removed for cleaner code.

3. frontend/src/container/CloudIntegrationPage/HeroSection/HeroSection.tsx:16

Draft comment:
Avoid using inline styles for the background image. Consider using a CSS class instead.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

4. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountActions.tsx:35

Draft comment:
Use a design token or predefined color constant instead of hardcoding the color value for the Check component.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

5. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountActions.tsx:99

Draft comment:
Use a design token or predefined color constant instead of hardcoding the color value for the ChevronDown component.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_bYZ7eg2kFD76dvLa

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-01-14T13:08:15Z

frontend/src/hooks/integrations/aws/useAccountSettingsModal.ts

+		if (checked) {
+			setSelectedRegions(['all']);
+		} else {
+			setSelectedRegions([]);


Consider retaining the previously selected regions when includeAllRegions is unchecked instead of clearing them entirely. This might provide a better user experience.

ahmadshaheer requested a review from YounixM as a code owner January 13, 2025 07:28

ellipsis-dev bot reviewed Jan 13, 2025

View reviewed changes

ahmadshaheer force-pushed the integrate-now-modal branch from 060fc10 to d044344 Compare January 14, 2025 08:56

ahmadshaheer force-pushed the account-settings branch from a976e18 to 294b1e1 Compare January 14, 2025 10:10

ellipsis-dev bot reviewed Jan 14, 2025

View reviewed changes

ahmadshaheer force-pushed the integrate-now-modal branch from d044344 to 3bae1b6 Compare January 14, 2025 12:58

ahmadshaheer added 8 commits January 14, 2025 17:30

feat: implement basic cloud account management UI in HeroSection

448c79c

feat: start working on integrate now modal UI

21da1a8

feat: get accounts from json-server API, and redirect Add new account…

2f231a5

… to the integrations modal

feat: integrate now modal UI

c288cce

feat: integrate now modal states and json server API integration

8812764

feat: account settings

fa72ce3

feat: service status UI

079ef1b

refactor: make account settings modal more readable and overall impro…

0a362df

…vements

ahmadshaheer force-pushed the account-settings branch from 294b1e1 to 0a362df Compare January 14, 2025 13:06

ellipsis-dev bot reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Integration: Account settings modal #6808

AWS Integration: Account settings modal #6808

ahmadshaheer commented Jan 13, 2025 •

edited by ellipsis-dev bot

Loading

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

ellipsis-dev bot left a comment

ellipsis-dev bot Jan 13, 2025

ellipsis-dev bot Jan 13, 2025

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

ellipsis-dev bot Jan 14, 2025

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

ellipsis-dev bot Jan 14, 2025

AWS Integration: Account settings modal #6808

Are you sure you want to change the base?

AWS Integration: Account settings modal #6808

Conversation

ahmadshaheer commented Jan 13, 2025 • edited by ellipsis-dev bot Loading

Summary

Related Issues / PR's

Screenshots

Affected Areas and Manually Tested Areas

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Jan 13, 2025

Choose a reason for hiding this comment

ellipsis-dev bot Jan 13, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Jan 14, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Jan 14, 2025

Choose a reason for hiding this comment

ahmadshaheer commented Jan 13, 2025 •

edited by ellipsis-dev bot

Loading