-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Get data from json server api data in service sections #6809
base: account-settings
Are you sure you want to change the base?
Get data from json server api data in service sections #6809
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to bc62edb in 1 minute and 9 seconds
More details
- Looked at
336
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/hooks/integrations/aws/useGetAccountServices.ts:11
- Draft comment:
ThegetAwsServices
function was modified to returnresponse.data.data
instead ofresponse.data.data.services
. Ensure that the data structure returned bygetAwsServices
matches the expected structure in this hook. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/hooks/integrations/aws/useServiceDetails.ts:15
- Draft comment:
Using!!serviceId
to enable the query is a common practice to ensure the query is only enabled whenserviceId
is truthy. This is a minor change fromBoolean(serviceId)
and is acceptable. - Reason this comment was not posted:
Confidence changes required:10%
TheuseServiceDetails
hook uses!!serviceId
to enable the query, which is a common practice to ensure the query is only enabled whenserviceId
is truthy. This is a minor change fromBoolean(serviceId)
and is acceptable.
3. frontend/src/pages/Integrations/CloudIntegrationPage/HeroSection/AccountActions.tsx:80
- Draft comment:
Avoid disablingreact-hooks/exhaustive-deps
. Ensure all dependencies are correctly listed in the dependency array to prevent unexpected behavior. - Reason this comment was not posted:
Comment did not seem useful.
4. frontend/src/pages/Integrations/CloudIntegrationPage/ServicesSection/ServiceDetails.tsx:57
- Draft comment:
Ensure thatserviceId
andaccountId
are correctly retrieved from the URL query parameters. Defaulting to empty strings or undefined is a good practice to ensure valid parameters are passed to the hook. - Reason this comment was not posted:
Confidence changes required:10%
InServiceDetails.tsx
, theserviceId
andaccountId
are retrieved from the URL query parameters. If these are not present, the hook defaults to empty strings or undefined. This is a good practice to ensure the hook is called with valid parameters.
5. frontend/src/pages/Integrations/CloudIntegrationPage/ServicesSection/ServicesList.tsx:19
- Draft comment:
Ensure thatactiveService
is initialized correctly. Ifservices
is not yet loaded, this might cause issues. Consider handling this case more gracefully. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_31kKuRDjzr1u7rGP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
a976e18
to
294b1e1
Compare
bc62edb
to
075fdd2
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Incremental review on 075fdd2 in 39 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountActions.tsx:74
- Draft comment:
TheuseEffect
hook is missingurlQuery
andnavigate
as dependencies. This can lead to unexpected behavior if these values change. Consider adding them to the dependency array. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
React hooks like useNavigate and custom hooks like useUrlQuery return stable references that don't change between renders. Adding them to the deps array would be unnecessary. The effect is specifically meant to sync with initialAccount changes, and the other values don't need to trigger the effect. The eslint-disable comment shows this was an intentional decision.
I could be wrong about the stability of useUrlQuery - without seeing its implementation, I can't be 100% certain it returns a stable reference.
Even if useUrlQuery returned an unstable reference, the current behavior is likely desired - we only want to update the URL when the initialAccount changes, not on every urlQuery change which could cause infinite loops.
The comment should be deleted. The dependency array is intentionally set up this way, and adding the suggested dependencies could cause unnecessary re-runs or even infinite loops.
Workflow ID: wflow_9Vd6Tqf6ASKa0b9T
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
294b1e1
to
0a362df
Compare
… to the integrations modal
075fdd2
to
7bb9a57
Compare
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Integrate AWS service data retrieval and display in frontend components using new hooks and updated API calls.
getAwsServices
inindex.ts
to return full data object instead of just services.AccountActions.tsx
: Update URL query and navigation when account changes.RegionForm.tsx
: Add form for selecting AWS regions with dynamic state updates.ServiceDetails.tsx
: Fetch and display service details dynamically usinguseServiceDetails
hook.ServicesList.tsx
: Fetch and display list of services dynamically usinguseGetAccountServices
hook.ServicesTabs.tsx
: Simplify service selection and display logic.useGetAccountServices
anduseServiceDetails
hooks for fetching AWS data.data.ts
andtypes.ts
to include new service configurations and types.This description was created by for 075fdd2. It will automatically update as commits are pushed.