-
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
feat: support updating subdomain for tenants #6745
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Reviewed everything up to f82e6de in 1 minute and 25 seconds
More details
- Looked at
916
lines of code in19
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/CustomDomainSettings/CustomDomainSettings.tsx:97
- Draft comment:
Consider defining the polling interval as a constant or making it configurable for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
TheuseEffect
hook inCustomDomainSettings.tsx
is responsible for polling the deployments data until the state becomes 'HEALTHY'. However, the polling interval is hardcoded to 3000 milliseconds. It would be better to define this interval as a constant or make it configurable, which would improve code readability and maintainability.
2. frontend/src/container/CustomDomainSettings/CustomDomainSettings.tsx:129
- Draft comment:
Ensuredns
is defined before constructing the URL to avoid copying an incorrect URL. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_qEkNWZcnrvN7RaeE
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.
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 17225b0 in 18 seconds
More details
- Looked at
39
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/pages/Settings/utils.ts:24
- Draft comment:
Consider adding a comment to explain the logic behindisCloudAccount && !isGatewayEnabled
condition, as it might not be immediately clear why ingestion settings are only pushed in this scenario. - Reason this comment was not posted:
Confidence changes required:50%
The refactoring of user role checks is a good practice for readability and maintainability. However, the logic forisCloudAccount && !isGatewayEnabled
is not explained in the PR description, and it might be worth checking if this is intended behavior.
Workflow ID: wflow_f2hJvma9He2INthX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
819ffd7
to
2082ba7
Compare
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 2082ba7 in 11 seconds
More details
- Looked at
39
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/pages/Settings/utils.ts:50
- Draft comment:
Consider includingisEECloudAccount
in the condition for addingcustomDomainSettings
, similar to theapiKeys
logic above, unless there's a specific reason not to. - Reason this comment was not posted:
Confidence changes required:50%
The refactoring of user role checks into variables is a good practice for readability and maintainability. However, the logic for addingcustomDomainSettings
is only checking forisCloudAccount
andisAdmin
. It might be worth considering ifisEECloudAccount
should also be included, similar to theapiKeys
logic.
Workflow ID: wflow_Dvm3yBR3HTZTAPh6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
https://www.loom.com/share/ef92e504cf494ca7a6b8402aed666b34?sid=ac45b035-d4db-447b-adab-42930f74175c
Important
Add support for updating tenant subdomains with new API endpoints, UI components, and route configurations.
CustomDomainSettings
component inCustomDomainSettings.tsx
for managing tenant subdomains.updateSubDomainAPI
inupdateSubDomain.ts
for updating subdomains.getDeploymentsData
ingetDeploymentsData.ts
to fetch deployment data.CUSTOM_DOMAIN_SETTINGS
route inroutes.ts
andconstants/routes.ts
.pageComponents.ts
to includeCustomDomainSettings
.CUSTOM_DOMAIN_SETTINGS
route toADMIN
inutils/permission/index.ts
.CustomDomainSettings.styles.scss
for the new component.Breadcrumbs
andDateTimeSelectionV2/config.ts
to include new route.custom_domain_settings
entries inlocales/en-GB/routes.json
andlocales/en-GB/titles.json
.This description was created by for 2082ba7. It will automatically update as commits are pushed.