Allow adding a data store at the top level #1004
Conversation
WalkthroughWalkthroughThe recent updates introduce enhancements to the frontend, particularly focusing on data store functionality. A new component, Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- spiffworkflow-frontend/src/components/DataStoreListTiles.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/ProcessGroupList.tsx (4 hunks)
Additional comments: 4
spiffworkflow-frontend/src/routes/ProcessGroupList.tsx (3)
- 17-17: The import of
DataStoreListTilesis correctly placed. Ensure that the component is utilized appropriately within this file.- 28-28: Adding permissions for
dataStoreListPathandprocessGroupListPathis a good practice for security. Verify that these permissions are correctly implemented in the backend.- 75-100: The layout modifications to include data store functionality are well-implemented. However, ensure that the
Stackcomponent'sorientationandgapproperties align with the design requirements. Also, verify that thehrefin the "Add a data store" button correctly handles the case when a parent group ID should be specified.spiffworkflow-frontend/src/components/DataStoreListTiles.tsx (1)
- 44-46: The update to construct
queryParamswithprocessGroup?.idis correctly implemented using optional chaining and nullish coalescing. This ensures that the query parameter includes the process group ID if available, providing a fallback to an empty string otherwise. Verify that the backend correctly handles the case whenprocess_group_identifieris an empty string.
| @@ -2,6 +2,7 @@ import { useEffect, useState } from 'react'; | |||
| import { useNavigate } from 'react-router-dom'; | |||
| import { | |||
| Button, | |||
| Stack, | |||
There was a problem hiding this comment.
Ensure the Stack import is grouped with other component imports from @carbon/react for better readability, following the consideration for future refactor mentioned by the user in a previous PR.
- Stack,
+ // Other imports from @carbon/react
+ StackCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Stack, | |
| // Other imports from @carbon/react | |
| Stack |
There was a problem hiding this comment.
Which pr comment are you referring to?
There was a problem hiding this comment.
@jbirddog: The comment I referred to is from a previous PR discussion in PR #919, where there was a consideration for future refactor regarding the suggestion to group related imports together in the ProcessInstanceListTable component for better readability. This was mentioned by the user jasquat and is captured in our learnings to ensure consistency in future reviews.
There was a problem hiding this comment.
Good to know, thanks.
There was a problem hiding this comment.
@jbirddog: You're welcome! If you have any more questions or need further assistance, feel free to ask.
/\_/\
( o.o )
> ^ <
| if (processGroup) { | ||
| queryParams = `${queryParams}&process_group_identifier=${processGroup.id}`; | ||
| } | ||
| const queryParams = `?per_page=1000&process_group_identifier=${ |
There was a problem hiding this comment.
maybe if you have a quick minute to omit process_group_identifier completely when id is not there, otherwise can merge this.
There was a problem hiding this comment.
Sadly it needs to be there, at least until I port the typeahead data store to have a location, since we still have to support requests coming in with no location vs the "" empty top level location...
This adds the UI support for creating a top level global data store.
Summary by CodeRabbit