Ri-7465 auto discovery layout #5181
Conversation
# Conflicts: # redisinsight/ui/src/components/connectivity-error/ConnectivityError.tsx # redisinsight/ui/src/components/inline-item-editor/InlineItemEditor.tsx # redisinsight/ui/src/components/item-list/components/delete-action/DeleteAction.tsx # redisinsight/ui/src/components/item-list/components/export-action/ExportAction.tsx # redisinsight/ui/src/components/query/query-actions/QueryActions.tsx # redisinsight/ui/src/components/query/query-card/QueryCardHeader/QueryCardHeader.tsx # redisinsight/ui/src/components/side-panels/panels/enablement-area/EnablementArea/components/InternalPage/InternalPage.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyHash/AddKeyHash.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyList/AddKeyList.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyReJSON/AddKeyReJSON.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeySet/AddKeySet.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyStream/AddKeyStream.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyString/AddKeyString.tsx # redisinsight/ui/src/pages/browser/components/add-key/AddKeyZset/AddKeyZset.tsx # redisinsight/ui/src/pages/browser/modules/key-details/components/rejson-details/components/edit-entire-item-action/EditEntireItemAction.tsx # redisinsight/ui/src/pages/browser/modules/key-details/components/set-details/add-set-members/AddSetMembers.tsx # redisinsight/ui/src/pages/database-analysis/components/header/Header.tsx # redisinsight/ui/src/pages/home/components/database-list-header/DatabaseListHeader.tsx # redisinsight/ui/src/pages/home/components/database-manage-tags-modal/ManageTagsModal.tsx # redisinsight/ui/src/pages/settings/components/cloud-settings/CloudSettings.tsx # redisinsight/ui/src/pages/settings/components/cloud-settings/components/user-api-keys-table/UserApiKeysTable.tsx # redisinsight/ui/src/pages/slow-log/components/Actions/Actions.tsx
# Conflicts: # redisinsight/ui/src/pages/home/components/manual-connection/manual-connection-form/ManualConnectionForm.tsx
pd-redis
requested review from
ArtemHoruzhenko,
DimoHG,
KrumTy,
dantovska,
pawelangelow and
valkirilov
as code owners
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Jit has detected 1 important finding in this PR that you should review.
The finding is detailed below as a comment.
It’s highly recommended that you fix this security issue before merge.
Repository Risks:
- Critical Severity Findings: Indicates that the resource has critical severity security findings that need immediate action.
- Internally Accessible: Accessible only within the internal network, reducing exposure to external threats but still requiring proper controls.
- Database Integration: Connects to a database, often involving sensitive data that must be securely managed.
- Production: Critical as it operates in a live production environment, directly impacting users and business operations.
Repository Context:
graph LR
GitHub$Repository_U23_redis/RedisInsight["GitHub Repository<br/>redis/RedisInsight"]:::GitHub$Repository
DBIntegration_U23_redis["DBIntegration<br/>redis"]:::DBIntegration
GitHub$Actions_U23_pipeline_U2D_build_U2D_windows_U2E_yml["GitHub Actions<br/>pipeline-build-windows.yml"]:::GitHub$Actions
GitHub$Repository_U23_redis/RedisInsight -- "Is accessible to" --> DBIntegration_U23_redis
GitHub$Repository_U23_redis/RedisInsight -- "Has" --> GitHub$Actions_U23_pipeline_U2D_build_U2D_windows_U2E_yml
Code Coverage - Frontend unit tests
Test suite run success5289 tests passing in 689 suites. Report generated by 🧪jest coverage report action from 71d37c6 |
|
Kristiyan Ivanov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
… user permissions Add platform specification and ensure redis user/group exist before running container
| }, | ||
| } | ||
|
|
||
| const backup12hInstances = RedisCloudInstanceFactoryOptionsBackupSchedule( |
There was a problem hiding this comment.
Nit: What is the difference between these 3 scenarios, because I don't see it reflected visually.
The only thing I've noticed to differentiate between the scenarios is the data in the table, which is a result of the factory, but that's all.
Backup Hourly
Backup Every 6 hours
Backup every 12 hours
There was a problem hiding this comment.
You will need to dig in to what these options mean, in any case these are just added for completeness
| columns: withoutModulesColumns, | ||
| }, | ||
| } | ||
| // Free vs Paid |
There was a problem hiding this comment.
Question: Do we have any underlying logic that depends on that, or is it just for completeness of the examples?
In the other examples, I see some visual changes in the table (like hiding the capabilities/options columns), but for this one, the only diff is on a factory level.
Free
Paid
There was a problem hiding this comment.
Yeah, just exhausting possible rendering variants. This is why you would use a story, to stick in a bunch of random data and see a bunch of display variations
|
|
||
| if (!shouldShowOptions) { | ||
| columns.splice( | ||
| columns.findIndex((col) => col.id === 'options'), |
There was a problem hiding this comment.
Nit: I suggest replacing these magic strings with a reusable enum, so that we can easily maintain them in the future. It will also help us apply changes and trace all the places where this functionality is used, if it's not just a regular string.
A place with all the available columns
export enum CloudDatabaseTableColumns {
// ...
Modules = 'modules',
Options = 'options'
// ...
}Then, in the column defintion, we can use the enum, instead of the hardcoded string
export const OptionsColumn = (
instances: InstanceRedisCloud[],
): ColumnDef<InstanceRedisCloud> => {
return {
id: CloudDatabaseTableColumns.Options,
header: 'Options',
...
}
}And later, whenever we do some checks based on the column ID, we can simply use the enums.
if (!shouldShowOptions) {
columns.splice(
columns.findIndex((col) => col.id === CloudDatabaseTableColumns.Options),There was a problem hiding this comment.
this was not the best approach to this specific task anyway, but I partially accept your comment
| ownerEmail: '[email protected]', | ||
| } | ||
|
|
||
| const instancesMock: InstanceRedisCloud[] = [ |
There was a problem hiding this comment.
I see you created RedisCloudInstanceFactory, so is it intentional that you don't use it here?
|
|
||
| const emptyColumns = colFactory([]) | ||
|
|
||
| const accountMock: RedisCloudAccount = { |
There was a problem hiding this comment.
Tip: It's a very small one, but it may be handy to introduce a factory for it as well, to follow the same patterns as the rest.
| const handleCopy = (text = '') => { | ||
| navigator.clipboard.writeText(text) | ||
| } | ||
| import { handleCopy } from 'uiSrc/utils' |
| }, | ||
| ]} | ||
| paginationEnabled={items.length > 10} | ||
| pageSizes={[5, 10, 25, 50, 100]} |
There was a problem hiding this comment.
Tip: I see this config in a few places already, maybe we should consider moving it out as a shared constant?
There was a problem hiding this comment.
Maybe, I didn't move it just to bring it to the discussion table. Probably a designer should say if we need to have it at all or rely on default implementation
| }, | ||
| } | ||
|
|
||
| const mockInstance = { |
There was a problem hiding this comment.
Can't we use RedisCloudInstanceFactory here, or the type of this mock is different, and we don't have a factory for it yet?
If that's the case, I would suggest creating one for it, like the rest we already have.
|
|
||
| import ManualConnectionForm from './' | ||
|
|
||
| const conn = { |
There was a problem hiding this comment.
Tip: One more candidate for a reusable factory.
| @@ -1,6 +1,12 @@ | |||
| FROM redislabs/redis:6.2.8-50 | |||
| FROM redislabs/redis:8.0.2-17 | |||
There was a problem hiding this comment.
I'm not sure how you handle it usually, but I saw plenty of other Dockerfiles with different versions for each stack, and I wonder do we want to keep the old version for some backward compatibility checks.
Or, it's not necessary in the current scenario.
There was a problem hiding this comment.
I think we should test with latest version mainly, but I will revert this one
There was a problem hiding this comment.
I am requesting changes (I don't do that usually) for a couple of reasons
- I do not agree with the way table columns have been handled. See my comment in column-definitions/AlertColumn.tsx for more context. In one of my PRs you had a suggestion for creating a more generic cell controller, which I did for RDI - check RdiInstancesListCell usage here. None of the bellow items is a component and none of them have tests, even though some should.
- I think this PR is very large. I see multiple tables and I don't think each one depends on the other. 1 PR per table would've been ideal. This is not a blocker at this stage, however.
- Storybook components - I think these should be in a separate PR as well, after the main one has been approved. Or we can agree not to review them. We do not currently have a rule as to when to add them even. This is something that should be discussed first.
| 0: { bg: '#293152' }, | ||
| 1: { bg: '#323e6c' }, | ||
| 2: { bg: '#465282' }, | ||
| 3: { bg: '#606c98' }, | ||
| 4: { bg: '#737fa8' }, | ||
| 5: { bg: '#8f99bc', fg: customFgDarkColor }, | ||
| 6: { bg: '#adb5d3', fg: customFgDarkColor }, | ||
| 7: { bg: '#cdd4ea', fg: customFgDarkColor }, | ||
| }, | ||
| light: { | ||
| 0: { bg: '#587AB2' }, | ||
| 1: { bg: '#6A8BC1' }, | ||
| 2: { bg: '#97B4E3', fg: customFgLightColor }, | ||
| 3: { bg: '#ADC5ED', fg: customFgLightColor }, |
There was a problem hiding this comment.
lots of custom colors, can you elaborate? Is this all custom for icons in db list options or it's more of a generic config for some icons?
There was a problem hiding this comment.
Yes, these are all custom colors. Provided that we update redis-ui, we will have the option to replace them with redis-ui colors, but until then (next pr at some point) we need this.
Having them co-located to the only place they are used will make the change simple once we update
| @@ -0,0 +1,108 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
for large PRs involving refactors it would be nice to have a note on what is just copied over and not new
There was a problem hiding this comment.
git shows what has been renamed and changed - sometimes, but ./index.ts became styles.ts because it is more smanticaly correct.
| const renderTitle = (title: React.ReactNode) => { | ||
| if (!title) { | ||
| return null | ||
| } | ||
| if (typeof title === 'string') { | ||
| return <Title size="XS">{title}</Title> | ||
| } | ||
| return title | ||
| } | ||
| export const HoverContent = ({ title, content }: RiTooltipContentProps) => { | ||
| return ( | ||
| <Col gap="s"> | ||
| {renderTitle(title)} | ||
| {content} |
There was a problem hiding this comment.
this looks a bit odd and now requires testing. Is it really used with that many title variants? I'd suggest refactoring it in a way that renderTitle can be dropped (lets write components and not functions that return jsx)
There was a problem hiding this comment.
It often causes console warnings for invalid dom nesting
|
|
||
| const DatabaseListOptions = ({ options }: Props) => { | ||
| const { theme } = useContext(ThemeContext) | ||
| const Tooltip = ({ |
There was a problem hiding this comment.
I think there's too much stuff going on in this file. Multiple components/styles and no tests changed is sus
| import { AlertStatusContent } from 'uiSrc/pages/autodiscover-cloud/components/AlertStatusContent' | ||
| import styles from 'uiSrc/pages/autodiscover-cloud/redis-cloud-subscriptions/styles.module.scss' | ||
|
|
||
| export const AlertColumn = (): ColumnDef<RedisCloudSubscription> => { |
There was a problem hiding this comment.
This is very misleading. export const AlertColumn inside a AlertColumn.tsx file - everyone would assume this to be a component. IMO the columns config should be a separated, the cell itself - this is a component and can be a separate file. See this as an example https://github.com/redis/RedisInsight/blob/main/redisinsight/ui/src/pages/rdi/home/components/rdi-instances-list/RdiInstancesList.config.tsx#L13
EDIT: Just so that I am more clear about the idea:
- Define all columns as a static array (example)
- You can later filter this columns array, if you want to show only some of the columns
- Cells - declare those as custom/reusable components that implement the same interface. They can have styles/tests/etc. They can access state/context and so on. (example)
This is a very robust solution with a clear separation of concerns. Generating columns run time and mixing them with components is not.
| const onSelectionChange = (selection: RowSelectionState) => { | ||
| // Map rowSelection state to the selected items list using uid as row id | ||
| const selectedItems = items.filter((i) => selection[`${i.uid}`]) | ||
| setSelection(selectedItems) | ||
| } |
There was a problem hiding this comment.
useMemo is preferable in this case
EDIT: to elaborate - this is only if you want to control row selection externally e.g. trigger an event that unselects all items if X is clicked. Without passing rowSelection you can't do that, but if you don't need it - you can keep current implementation
| if (instancesAdded.length) { | ||
| return ( | ||
| <RedisClusterDatabasesResult | ||
| instances={instancesAdded || []} |
There was a problem hiding this comment.
instead of using new [], declare export const EMPTY_ARRAY: any[] = [] somewhere and use that
preferably use it in the hook even
| paginationEnabled={items.length > 10} | ||
| stripedRows | ||
| pageSizes={[5, 10, 25, 50, 100]} | ||
| emptyState={() => ( | ||
| <Col centered full> | ||
| <FlexItem padding={13}> | ||
| <Text size="L">{message}</Text> | ||
| </FlexItem> |
There was a problem hiding this comment.
reuse, also 10 - extract as const too
| const [columns, columnsResult] = useMemo( | ||
| () => colFactory(instances), | ||
| [instances], |
There was a problem hiding this comment.
you don't need to recreate all columns if instances change, there's only one column's cell that depends on it - use context/redux instead
| # redis enterprise | ||
| redis-enterprise: | ||
| logging: *logging | ||
| platform: linux/amd64 |
There was a problem hiding this comment.
should this be part of this PR?
| @@ -1,4 +1,4 @@ | |||
| FROM redislabs/redis:6.2.8-50 | |||
| FROM redislabs/redis:8.0.2-17 | |||
There was a problem hiding this comment.
Security control: Docker Scan
Image User Should Not Be 'Root'
Running containers with 'root' user can lead to a container escape situation. It is a best practice to run containers as non-root users, which can be done by adding a 'USER' statement to the Dockerfile.
Severity: HIGH
Fix suggestion:
This fix suggestion was generated by Jit. Please note that the suggestion might not always fit every use case. It is highly recommended that you check and review it before merging.
Suggestion guidelines
- First of all, check if your container is running as a root user. In most of the cases, you can do it by running a command like this:
docker run <image> whoami. If it returnsroot, then you should consider using a non-root user, by following one of the next steps:- If a non-root user already exists in your container, consider using it.
- If not, you can create a new user by adding a
USERcommand to the Dockerfile, with a non-root user as argument, for example:USER <non-root-user-name>.
| FROM redislabs/redis:8.0.2-17 | |
| FROM redislabs/redis:8.0.2-17 | |
| RUN addgroup --system <group> | |
| RUN adduser --system <user> --ingroup <group> | |
| USER <user>:<group> | |
Why should you fix this issue?
This Dockerfile introduces a container vulnerability. In a production environment, using insecure container configurations or outdated base images can lead to significant security risks. If an attacker exploits a vulnerability in the container, it could compromise the entire application or lead to unauthorized access.
Jit Bot commands and options (e.g., ignore issue)
You can trigger Jit actions by commenting on this PR review:
#jit_ignore_fpIgnore and mark this specific single instance of finding as “False Positive”#jit_ignore_acceptIgnore and mark this specific single instance of finding as “Accept Risk”#jit_ignore_type_in_fileIgnore any finding of type "Image user should not be 'root'" in tests/e2e/rte/redis-enterprise/Dockerfile; future occurrences will also be ignored.#jit_undo_ignoreUndo ignore command
Extract complex cell renderers from cloud column definitions into 5 standalone components (Alert, Database, Endpoint, MessageResult, Subscription) with types and tests. Handle optional props in MessageResultCell for undefined status/message values.
…iscovery hooks into hooks/utils/types with comprehensive tests
…covery stories and consolidate duplicate stories
… text for empty values
…isCloudInstanceFactory
What
This PR refactors the Redis Cloud and Redis Cluster database discovery UI by extracting inline table column definitions
into dedicated, reusable components. These changes bring the current implementation closer to Figma designs while
improving code maintainability, testability, and consistency across the application.
Key Changes:
1. Column Definition Extraction
2. Custom Hook Extraction
3. Storybook Stories
RedisClusterDatabases(15+ variants)RedisCloudDatabaseswith different statesRedisCloudSubscriptionswith multipage examplesRedisCloudDatabasesResultwith success/error states4. UI/UX Improvements (Aligned with Figma Designs)
DatabaseListOptionscomponent with better styling and stories to match design specsMessageBarcomponent with variant support and comprehensive testsHoverContentto not wrap non-text titles per design guidelinesClusterConnectionFormlayout to align with Figma mockupsTech Decisions:
ColumnDefCopyBtn,CopyTextContainer, andCellTextcomponents for consistencyTesting
Manual Testing:
tests/e2e/rte/redis-enterprisefolderAutomation Tests:
MessageBarcomponent tests covering all variants and statesVisual Changes:
Screenshots/Recordings:
Redis Cloud
Screen.Recording.2025-11-12.at.16.15.39.mov
Sentinel Cluster
Screen.Recording.2025-11-12.at.16.17.24.mov
Redis Cluster
Screen.Recording.2025-11-12.at.16.16.37.mov