[Nu-1890] hide categories from a scenarios list and more scenario details when only one category is available #7183
Conversation
…ls when only one category is available
Dzuming
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce several enhancements primarily centered around state management and component rendering in a React application. A new custom hook, Additionally, the Overall, the changes reflect a shift towards a more modular and maintainable approach, utilizing hooks for shared logic and enhancing type safety across the application. Sequence Diagram(s)sequenceDiagram
participant A as AddProcessDialog
participant B as MoreScenarioDetailsDialog
participant C as CategoryDetails
participant D as FiltersPart
participant E as FirstLine
participant F as TablePart
participant G as useGetAllCombinations
participant H as useScenariosWithCategoryVisible
A->>G: useGetAllCombinations()
G-->>A: {allCombinations, engineSetupErrors, isAllCombinationsLoading}
A->>B: Pass data to MoreScenarioDetailsDialog
B->>G: useGetAllCombinations()
G-->>B: {isAllCombinationsLoading, isCategoryFieldVisible}
B->>C: Pass data to CategoryDetails
C->>G: useGetAllCombinations()
G-->>C: {isAllCombinationsLoading, isCategoryFieldVisible}
D->>H: useScenariosWithCategoryVisible()
H-->>D: {withCategoriesVisible}
E->>H: useScenariosWithCategoryVisible()
H-->>E: {withCategoriesVisible}
F->>H: useScenariosWithCategoryVisible()
H-->>F: {withCategoriesVisible}
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)docs/Changelog.md (1)
The changelog entry accurately describes the changes made in PR #7183 to hide category-related UI elements when only one category is available. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx (1)
Line range hint
19-19: Fix the suspicious trailing forward slash in Typography.The Typography component contains a trailing forward slash that appears to be a typo:
<Typography variant={"body2"}>{scenario.processCategory} /</Typography>Consider removing the trailing slash:
-<Typography variant={"body2"}>{scenario.processCategory} /</Typography> +<Typography variant={"body2"}>{scenario.processCategory}</Typography>designer/client/src/components/useGetAllCombinations.ts (3)
5-9: Consider enhancing type safety of the Props interfaceThe Props interface could benefit from additional type safety.
Consider these improvements:
interface Props { - processCategory: string; + processCategory: string | null; - processingMode: ProcessingMode; + processingMode: ProcessingMode | undefined; - processEngine: string; + processEngine: string | null; }
10-13: Consider using a more explicit initial state for engineSetupErrorsThe empty object as initial state is fine, but being more explicit about the expected structure could improve code clarity.
Consider this improvement:
- const [engineSetupErrors, setEngineSetupErrors] = useState<Record<string, string[]>>({}); + const [engineSetupErrors, setEngineSetupErrors] = useState<Record<string, string[]>>(() => ({}));
15-22: Consider memoizing the value object to prevent unnecessary re-rendersThe value object passed to useProcessFormDataOptions is recreated on every render, which could trigger unnecessary re-computations.
Consider this optimization:
+ const value = useMemo(() => ({ + processCategory, + processingMode, + processEngine, + }), [processCategory, processingMode, processEngine]); + const { isCategoryFieldVisible } = useProcessFormDataOptions({ allCombinations, - value: { - processCategory, - processingMode, - processEngine, - }, + value, });designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx (1)
115-116: Consider handling null/undefined category values.The current implementation might group null/undefined categories together. Consider explicitly handling these cases:
- const withCategoriesVisible = Object.keys(groupBy(data, (scenario) => scenario.processCategory)).length > 1; + const withCategoriesVisible = Object.keys(groupBy(data, (scenario) => scenario.processCategory || 'uncategorized')).length > 1;designer/submodules/packages/components/src/scenarios/list/item.tsx (1)
68-73: Consider these maintainability improvements.While the conditional rendering logic is correct, consider these enhancements:
- Move the padding values to theme constants for consistency
- Consider internationalizing the separator character
+ // Add to your theme constants + const SPACING = { + categoryPadding: 8, + }; - <span style={{ paddingLeft: 8, paddingRight: 8 }}>/</span> + <span style={{ paddingLeft: SPACING.categoryPadding, paddingRight: SPACING.categoryPadding }}> + {t("scenario.categorySeparator", "/")} + </span>designer/client/src/components/modals/MoreScenarioDetailsDialog.tsx (1)
42-46: Consider adding error handlingThe hook usage should include error handling to gracefully handle cases where the combinations fetch fails.
Consider destructuring and handling potential errors:
const { isAllCombinationsLoading, isCategoryFieldVisible, error } = useGetAllCombinations({...}); // Add error handling if (error) { // Handle error appropriately console.error('Failed to load combinations:', error); }designer/submodules/packages/components/src/scenarios/list/tablePart.tsx (1)
Line range hint
24-44: Consider explicit type handling for conditional columnsWhile the current implementation works, consider making the type safety more explicit when dealing with conditional columns.
- const availableColumns: Columns<RowType | undefined> = [ + const availableColumns: Array<Columns<RowType>[number] | undefined> = [designer/client/src/components/AddProcessForm.tsx (2)
Line range hint
171-199: Implementation doesn't match PR objectiveThe current implementation only hides categories when the array is empty (
isEmpty(categories)). According to the PR objective, categories should also be hidden when there's exactly one category available.Consider updating the condition to:
-{!isEmpty(categories) && ( +{categories.length > 1 && (
Line range hint
82-117: Consider accessibility improvements for form controlsWhile the UI implementation is solid, consider enhancing accessibility by:
- Adding
aria-labelattributes to radio controls- Ensuring proper focus management
- Adding keyboard navigation support
Example improvement for radio controls:
<CustomRadio disabled={processingModes.every((processingMode) => processingMode !== ProcessingMode.streaming)} label={t("addProcessForm.label.streaming", "Streaming")} value={ProcessingMode.streaming} Icon={StreamingIcon} active={value.processingMode === ProcessingMode.streaming} + aria-label={t("addProcessForm.aria.streaming", "Select streaming processing mode")} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
designer/client/src/components/AddProcessDialog.tsx(3 hunks)designer/client/src/components/AddProcessForm.tsx(1 hunks)designer/client/src/components/modals/MoreScenarioDetailsDialog.tsx(3 hunks)designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx(1 hunks)designer/client/src/components/useGetAllCombinations.ts(1 hunks)designer/submodules/packages/components/src/scenarios/filters/filtersPart.tsx(4 hunks)designer/submodules/packages/components/src/scenarios/list/item.tsx(2 hunks)designer/submodules/packages/components/src/scenarios/list/tablePart.tsx(3 hunks)designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx(2 hunks)docs/Changelog.md(1 hunks)
🔇 Additional comments (25)
designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx (2)
1-4: LGTM! Clean imports that align with component needs.
7-11: Verify the category visibility logic.
Let's verify that the useGetAllCombinations hook correctly determines category visibility based on the PR's objective of hiding categories when only one is available.
✅ Verification successful
Let me analyze the implementation of category visibility logic by checking the useProcessFormDataOptions hook since it's used by useGetAllCombinations to determine category visibility.
Based on the implementation and tests, I can now provide a definitive response about the category visibility logic:
Category visibility logic correctly implemented.
The useProcessFormDataOptions hook properly implements the category visibility logic by:
- Computing unique categories from all available combinations
- Setting
isCategoryFieldVisible = categories.length > 1, which directly aligns with the requirement to hide categories when only one is available - Test coverage confirms this behavior with a specific test case that verifies
isCategoryFieldVisibleis false when only one category exists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of category visibility logic
# Check the hook implementation
echo "Checking useGetAllCombinations implementation:"
ast-grep --pattern 'export const useGetAllCombinations = $_' -A 20
# Check for other usages of the hook to ensure consistent behavior
echo "\nChecking other usages of useGetAllCombinations:"
rg -A 5 "useGetAllCombinations"
# Check for category-related logic
echo "\nChecking category-related logic:"
rg -A 5 "processCategory|isCategoryFieldVisible"
Length of output: 144305
Script:
#!/bin/bash
# Check the implementation of useProcessFormDataOptions hook
echo "Checking useProcessFormDataOptions implementation:"
ast-grep --pattern 'const useProcessFormDataOptions = $_' -A 30
# Check tests for useProcessFormDataOptions
echo "\nChecking useProcessFormDataOptions tests:"
rg -A 10 "should return isCategoryFieldVisible false when there is only one category"
Length of output: 42197
designer/client/src/components/useGetAllCombinations.ts (2)
36-37: LGTM! Clean and well-structured return value
The hook returns all necessary values in a clear and organized manner.
1-37: Verify integration with consuming components
The hook implementation aligns with the PR objectives of hiding categories when only one is available. However, let's verify its integration with the consuming components.
✅ Verification successful
Integration verified successfully
The hook is properly integrated across the codebase:
CategoryDetails.tsxcorrectly usesisCategoryFieldVisibleto conditionally render the categoryAddProcessDialog.tsxuses it for form validation and field visibilityMoreScenarioDetailsDialog.tsxproperly handles the visibility of category details- Unit tests in
useProcessFormDataOptions.test.tsverify the visibility logic for both single and multiple category scenarios
The implementation and its usage align perfectly with the PR's objective of hiding categories when only one is available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hook's usage across components
# Check components using this hook
echo "Components using useGetAllCombinations:"
rg -l "useGetAllCombinations" --type ts --type tsx
# Verify isCategoryFieldVisible usage
echo "\nCategory visibility logic usage:"
rg "isCategoryFieldVisible" --type ts --type tsx -A 3
Length of output: 328
Script:
#!/bin/bash
# Let's try without specifying file types and use more generic search
echo "Components using useGetAllCombinations:"
rg -l "useGetAllCombinations"
echo -e "\nCategory visibility logic usage:"
rg "isCategoryFieldVisible" -A 3
Length of output: 9775
designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx (3)
10-10: LGTM!
The groupBy utility from lodash is an appropriate choice for grouping scenarios by category.
112-121: LGTM! Clean and efficient implementation.
The hook effectively determines category visibility by counting unique categories, with proper memoization for performance optimization.
112-122: Verify consistent usage across all category-related components.
The hook implementation aligns with the PR objective. Let's verify it's properly integrated in all relevant components:
✅ Verification successful
Hook usage is properly integrated across all relevant components
The verification shows that the useScenariosWithCategoryVisible hook is consistently used in all the necessary category-related components:
scenarios/filters/filtersPart.tsxscenarios/list/tablePart.tsxscenarios/list/item.tsx
These components represent all the main UI parts that need to handle category visibility, and each correctly imports and uses the hook with the withCategoriesVisible flag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might need to use this hook
echo "Components potentially handling categories:"
rg -l "processCategory|CategoryDetails|FiltersPart|TablePart" --type tsx
echo "\nCurrent usage of the new hook:"
rg "useScenariosWithCategoryVisible" --type tsx
Length of output: 350
Script:
#!/bin/bash
# Search for components that might need to use this hook, adjusting file extension
echo "Components potentially handling categories:"
rg -l "processCategory|CategoryDetails|FiltersPart|TablePart" --type ts --type tsx --type jsx --type js
echo -e "\nCurrent usage of the new hook:"
rg "useScenariosWithCategoryVisible" --type ts --type tsx --type jsx --type js
Length of output: 414
Script:
#!/bin/bash
# Search for components that might need to use this hook without specifying file types
echo "Components potentially handling categories:"
rg -l "processCategory|CategoryDetails|FiltersPart|TablePart"
echo -e "\nCurrent usage of the new hook:"
rg "useScenariosWithCategoryVisible"
# Also check the directory structure to understand file organization
echo -e "\nRelevant files in components directory:"
fd . designer/submodules/packages/components/src
Length of output: 16100
designer/submodules/packages/components/src/scenarios/list/item.tsx (3)
16-16: LGTM! Clear and purposeful import.
The import of useScenariosWithCategoryVisible hook aligns with the PR objective and follows consistent import patterns.
64-64: LGTM! Proper hook usage.
The hook is correctly used at the component level and follows React hooks best practices.
64-73: Verify the hook implementation.
Let's ensure the useScenariosWithCategoryVisible hook correctly determines visibility based on category count.
✅ Verification successful
Let's gather more information about the hook implementation since we found the files where it's used.
Hook implementation is correct and well-designed
The useScenariosWithCategoryVisible hook is properly implemented:
- It correctly determines visibility based on category count using
groupByfrom lodash - Shows categories only when there are multiple distinct categories (
length > 1) - Uses proper memoization with
useMemoto prevent unnecessary recalculations - Is consistently used across components (list item, table part, and filters)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the hook implementation to verify the logic
ast-grep --pattern 'export function useScenariosWithCategoryVisible() {
$$$
}'
# Search for other usages to ensure consistent behavior
rg -l 'useScenariosWithCategoryVisible'
Length of output: 417
Script:
#!/bin/bash
# Get the hook implementation from useScenariosQuery.tsx
cat designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx
# Get the context of other usages
rg -A 5 -B 5 'useScenariosWithCategoryVisible' designer/submodules/packages/components/src/scenarios/list/tablePart.tsx designer/submodules/packages/components/src/scenarios/filters/filtersPart.tsx
Length of output: 10056
designer/client/src/components/modals/MoreScenarioDetailsDialog.tsx (1)
13-13: LGTM!
Clean import statement following standard practices.
designer/client/src/components/AddProcessDialog.tsx (4)
2-2: LGTM! Clean refactoring of imports and hook introduction.
The introduction of useGetAllCombinations hook and removal of direct ScenarioParametersCombination import improves code organization by centralizing the combinations logic.
Also applies to: 6-6, 15-15
26-31: LGTM! Type-safe state initialization.
The explicit type casting of processingMode to ProcessingMode improves type safety. The state structure correctly maintains processCategory for internal use even when it might be hidden from the UI.
Line range hint 58-59: LGTM! Smart conditional validation.
The validation logic correctly adapts to the visibility state of the category field, which aligns well with the PR objective of hiding categories when appropriate.
40-44: Verify the hook's behavior for single category scenarios.
The integration of useGetAllCombinations looks good. However, we should verify that it correctly handles the PR's main objective of hiding categories when only one is available.
✅ Verification successful
Hook correctly implements category hiding functionality
The verification confirms that the hook's behavior aligns perfectly with the PR's objective of hiding categories when only one is available:
useProcessFormDataOptionsexplicitly setsisCategoryFieldVisible = categories.length > 1AddProcessDialogcorrectly uses this flag by conditionally passing categories:categories={isCategoryFieldVisible ? categories : []}- The implementation ensures the category field is hidden when only one category is available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of category hiding logic in useGetAllCombinations
# Check if the hook implementation includes logic for hiding single categories
ast-grep --pattern 'export function useGetAllCombinations' -A 20
# Check for any conditional rendering logic related to categories
rg -l "isCategoryFieldVisible|categories\.length === 1" --type ts
Length of output: 511
Script:
#!/bin/bash
# Let's check the actual implementation of useGetAllCombinations and related components
# Check the hook implementation
cat designer/client/src/components/useGetAllCombinations.ts
# Check AddProcessDialog for category visibility logic
cat designer/client/src/components/AddProcessDialog.tsx
# Check useProcessFormDataOptions for category-related logic
cat designer/client/src/components/useProcessFormDataOptions.ts
Length of output: 11321
designer/submodules/packages/components/src/scenarios/filters/filtersPart.tsx (4)
5-5: LGTM: Clean import addition
The new hook import is properly grouped with related query hooks.
24-24: LGTM: Clean hook integration
The hook is properly integrated at the component level, following React best practices for data fetching.
106-118: LGTM: Well-implemented conditional rendering
The category filter menu is properly wrapped with conditional rendering while maintaining consistency with other filters and preserving event tracking functionality.
39-39: Verify handling of undefined labels
While the dependency is correctly added, let's verify the handling of undefined labels to prevent potential issues.
Consider adding a default empty array to prevent potential issues:
-availableLabels?.labels
+availableLabels?.labels || []✅ Verification successful
Undefined labels are properly handled
The code already implements proper handling of undefined labels with a fallback to an empty array:
label: (availableLabels?.labels || []).map((name) => ({ name }))The optional chaining in the dependency array is consistent with its usage in the memo implementation, making the dependency addition both necessary and correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for label-related error handling in the codebase
rg -A 3 "availableLabels\?.labels" --type typescript
Length of output: 89
Script:
#!/bin/bash
# Search for label-related patterns with ripgrep, without file type restriction
rg -A 5 "availableLabels\?.labels"
# Also search for the filterableValues memo implementation
ast-grep --pattern 'const filterableValues = useMemo($$$)'
Length of output: 4535
designer/submodules/packages/components/src/scenarios/list/tablePart.tsx (3)
14-14: LGTM: Clean hook integration
The integration of useScenariosWithCategoryVisible hook is clean and follows React best practices.
Also applies to: 22-22
116-117: LGTM: Efficient column filtering
The use of filter to remove undefined entries is clean and efficient. The useMemo dependencies are correctly specified to ensure optimal performance.
Line range hint 149-149: Verify filter behavior when categories are hidden
The CATEGORY filter rule remains active even when the category column is hidden. Please verify that:
- The category filter UI is properly hidden when
withCategoriesVisibleis false - The filter cannot be accidentally activated through other means when categories are hidden
✅ Verification successful
Let me try a different search strategy to find category-related UI components and filter controls.
Let me try one more search strategy without file type restrictions to find the relevant code.
Category filter is properly handled when categories are hidden
Based on the code analysis, the category filter behavior is correctly implemented:
-
The
withCategoriesVisibleflag is properly used to conditionally render both the category filter UI and column:- In
filtersPart.tsx, the category filter menu is wrapped in{withCategoriesVisible && (...)}condition - In
tablePart.tsx, the category column is only added whenwithCategoriesVisibleis true - In
item.tsx, the category display is also conditionally rendered
- In
-
The
withCategoriesVisibleflag is computed based on actual data inuseScenariosWithCategoryVisible:- Only shows categories when there's more than one unique category in the data
- This ensures the filter UI is hidden when categories are not relevant
-
The filter rules in
filterRules.tsxshow that the category filter is passive when not used:- The rule
CATEGORY: (row, value) => !value?.length || ...returns true when no category filter is active - This means hidden categories won't affect the filtering results
- The rule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any UI components or filter controls that might still reference category filtering
rg -l "CATEGORY" --type ts --type tsx | xargs ast-grep --pattern 'filterKey="CATEGORY"'
Length of output: 8708
Script:
#!/bin/bash
# Search for category-related UI components and filter controls
rg -A 5 "ScenariosFiltersModelType.CATEGORY" --type ts --type tsx
# Also search for withCategoriesVisible usage
rg -A 5 "withCategoriesVisible" --type ts --type tsx
Length of output: 174
Script:
#!/bin/bash
# Search for category-related UI components and filter controls without file type restrictions
rg -A 5 "ScenariosFiltersModelType.CATEGORY"
# Search for withCategoriesVisible usage
rg -A 5 "withCategoriesVisible"
# Search for filter-related components
rg -A 5 "ScenariosFiltersModelType"
Length of output: 15581
designer/client/src/components/AddProcessForm.tsx (2)
22-22: LGTM: Type safety improvement
Good improvement changing processingMode from string to ProcessingMode enum type, which ensures type safety and prevents invalid processing mode assignments.
Line range hint 124-127: LGTM: Robust validation implementation
The validation implementation is well-structured with:
- Proper touch state tracking
- Consistent error handling across fields
- Field-specific error filtering
Also applies to: 186-190
docs/Changelog.md (1)
103-103: LGTM! The changelog entry matches the PR objectives.
The changelog entry correctly documents the new feature to hide categories when only one category is available, which aligns with the PR objectives.
| useEffect(() => { | ||
| setIsAllCombinationsLoading(true); | ||
| HttpService.fetchScenarioParametersCombinations() | ||
| .then((response) => { | ||
| setAllCombinations(response.data.combinations); | ||
| setEngineSetupErrors(response.data.engineSetupErrors); | ||
| }) | ||
| .finally(() => { | ||
| setIsAllCombinationsLoading(false); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Add error handling and cleanup to the fetch effect
The current implementation lacks error handling and cleanup for the fetch request.
Consider these improvements:
useEffect(() => {
+ const abortController = new AbortController();
setIsAllCombinationsLoading(true);
- HttpService.fetchScenarioParametersCombinations()
+ HttpService.fetchScenarioParametersCombinations({ signal: abortController.signal })
.then((response) => {
setAllCombinations(response.data.combinations);
setEngineSetupErrors(response.data.engineSetupErrors);
})
+ .catch((error) => {
+ if (!error.name === 'AbortError') {
+ console.error('Failed to fetch scenario parameters:', error);
+ setEngineSetupErrors(prev => ({
+ ...prev,
+ fetch: ['Failed to load scenario parameters']
+ }));
+ }
+ })
.finally(() => {
setIsAllCombinationsLoading(false);
});
+ return () => {
+ abortController.abort();
+ };
}, []);📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| setIsAllCombinationsLoading(true); | |
| HttpService.fetchScenarioParametersCombinations() | |
| .then((response) => { | |
| setAllCombinations(response.data.combinations); | |
| setEngineSetupErrors(response.data.engineSetupErrors); | |
| }) | |
| .finally(() => { | |
| setIsAllCombinationsLoading(false); | |
| }); | |
| }, []); | |
| useEffect(() => { | |
| const abortController = new AbortController(); | |
| setIsAllCombinationsLoading(true); | |
| HttpService.fetchScenarioParametersCombinations({ signal: abortController.signal }) | |
| .then((response) => { | |
| setAllCombinations(response.data.combinations); | |
| setEngineSetupErrors(response.data.engineSetupErrors); | |
| }) | |
| .catch((error) => { | |
| if (!error.name === 'AbortError') { | |
| console.error('Failed to fetch scenario parameters:', error); | |
| setEngineSetupErrors(prev => ({ | |
| ...prev, | |
| fetch: ['Failed to load scenario parameters'] | |
| })); | |
| } | |
| }) | |
| .finally(() => { | |
| setIsAllCombinationsLoading(false); | |
| }); | |
| return () => { | |
| abortController.abort(); | |
| }; | |
| }, []); |
| const { isAllCombinationsLoading, isCategoryFieldVisible } = useGetAllCombinations({ | ||
| processCategory: scenario.processCategory, | ||
| processingMode: scenario.processingMode, | ||
| processEngine: scenario.engineSetupName, | ||
| }); |
There was a problem hiding this comment.
Fix incorrect visibility logic
The current implementation shows the category when isAllCombinationsLoading is true, which seems incorrect for two reasons:
- Using a loading state to control visibility could cause unwanted UI flashing
- This appears to be the opposite of the PR's objective to hide categories when there's only one available
Consider updating the logic to:
- const { isAllCombinationsLoading, isCategoryFieldVisible } = useGetAllCombinations({
+ const { isCategoryFieldVisible } = useGetAllCombinations({
processCategory: scenario.processCategory,
processingMode: scenario.processingMode,
processEngine: scenario.engineSetupName,
});Committable suggestion skipped: line range outside the PR's diff.
| {isAllCombinationsLoading && isCategoryFieldVisible && ( | ||
| <ItemWrapperStyled> | ||
| <ItemLabelStyled>{i18next.t("scenarioDetails.label.category", "Category")}</ItemLabelStyled> | ||
| <Typography variant={"caption"}>{scenario.processCategory}</Typography> | ||
| </ItemWrapperStyled> | ||
| )} |
There was a problem hiding this comment.
💡 Codebase verification
Loading state should not affect category visibility
The review comment is correct. After examining the useGetAllCombinations hook implementation, I can confirm that:
isAllCombinationsLoadingis just a loading indicator for the API call that fetches combinations dataisCategoryFieldVisibleis determined independently byuseProcessFormDataOptionsbased on the current combinations and form values- The loading state should not affect whether the category field is displayed
The current condition isAllCombinationsLoading && isCategoryFieldVisible is incorrect as it will hide the category field when the data is not loading, even if isCategoryFieldVisible is true. The category's visibility should depend solely on isCategoryFieldVisible.
🔗 Analysis chain
Simplify category visibility condition
The category visibility should depend solely on isCategoryFieldVisible, not the loading state.
Apply this change:
- {isAllCombinationsLoading && isCategoryFieldVisible && (
+ {isCategoryFieldVisible && (
<ItemWrapperStyled>
<ItemLabelStyled>{i18next.t("scenarioDetails.label.category", "Category")}</ItemLabelStyled>
<Typography variant={"caption"}>{scenario.processCategory}</Typography>
</ItemWrapperStyled>
)}Let's verify the behavior of useGetAllCombinations hook:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation and usage of useGetAllCombinations
ast-grep --pattern 'export function useGetAllCombinations' -A 20
Length of output: 66
Script:
#!/bin/bash
# Let's try searching for any usage of useGetAllCombinations
rg "useGetAllCombinations" -A 3
# Also search for any hooks related to combinations
rg "useCombinations?" -A 3
# Let's also check the component's implementation
fd "MoreScenarioDetailsDialog" --exec cat {}
Length of output: 9311
Script:
#!/bin/bash
# Let's check the implementation of useGetAllCombinations hook
cat designer/client/src/components/useGetAllCombinations.ts
Length of output: 1512
|
created: #7185 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx (1)
58-72: Consider adding error type information for better type safety.The implementation looks good and follows the established patterns. However, it would be beneficial to explicitly type the error response for better error handling.
Consider updating the type definition:
-export function useScenarioParametersCombinationsQuery(): UseQueryResult<ScenarioParametersCombinations> { +export function useScenarioParametersCombinationsQuery(): UseQueryResult<ScenarioParametersCombinations, Error> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx(3 hunks)
🔇 Additional comments (2)
designer/submodules/packages/components/src/scenarios/useScenariosQuery.tsx (2)
5-5: LGTM!
The import statement for ScenarioParametersCombinations is correctly placed and necessary for the new functionality.
123-123: LGTM!
The added comma improves code formatting consistency.
| export function useScenariosWithCategoryVisible(): { withCategoriesVisible: boolean } { | ||
| const parametersCombinations = useScenarioParametersCombinationsQuery(); | ||
| return useMemo(() => { | ||
| const { data } = parametersCombinations; | ||
| const combinations = data?.combinations || []; | ||
|
|
||
| const withCategoriesVisible = Object.keys(groupBy(combinations, (combination) => combination.category)).length > 1; | ||
|
|
||
| return { | ||
| withCategoriesVisible, | ||
| }; | ||
| }, [parametersCombinations]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider exposing loading and error states for better UX handling.
While the implementation is functionally correct, consider these improvements for better error handling and loading state management:
-export function useScenariosWithCategoryVisible(): { withCategoriesVisible: boolean } {
+export function useScenariosWithCategoryVisible(): {
+ withCategoriesVisible: boolean;
+ isLoading: boolean;
+ error: Error | null;
+} {
const parametersCombinations = useScenarioParametersCombinationsQuery();
return useMemo(() => {
- const { data } = parametersCombinations;
+ const { data, isLoading, error } = parametersCombinations;
const combinations = data?.combinations || [];
- const withCategoriesVisible = Object.keys(groupBy(combinations, (combination) => combination.category)).length > 1;
+ const MINIMUM_CATEGORIES_FOR_VISIBILITY = 1;
+ const withCategoriesVisible = Object.keys(groupBy(combinations, (combination) => combination.category)).length > MINIMUM_CATEGORIES_FOR_VISIBILITY;
return {
withCategoriesVisible,
+ isLoading,
+ error,
};
}, [parametersCombinations]);
}This change will:
- Expose loading and error states to consumers
- Make the visibility threshold more explicit with a named constant
- Allow components to handle loading and error states appropriately
📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useScenariosWithCategoryVisible(): { withCategoriesVisible: boolean } { | |
| const parametersCombinations = useScenarioParametersCombinationsQuery(); | |
| return useMemo(() => { | |
| const { data } = parametersCombinations; | |
| const combinations = data?.combinations || []; | |
| const withCategoriesVisible = Object.keys(groupBy(combinations, (combination) => combination.category)).length > 1; | |
| return { | |
| withCategoriesVisible, | |
| }; | |
| }, [parametersCombinations]); | |
| } | |
| export function useScenariosWithCategoryVisible(): { | |
| withCategoriesVisible: boolean; | |
| isLoading: boolean; | |
| error: Error | null; | |
| } { | |
| const parametersCombinations = useScenarioParametersCombinationsQuery(); | |
| return useMemo(() => { | |
| const { data, isLoading, error } = parametersCombinations; | |
| const combinations = data?.combinations || []; | |
| const MINIMUM_CATEGORIES_FOR_VISIBILITY = 1; | |
| const withCategoriesVisible = Object.keys(groupBy(combinations, (combination) => combination.category)).length > MINIMUM_CATEGORIES_FOR_VISIBILITY; | |
| return { | |
| withCategoriesVisible, | |
| isLoading, | |
| error, | |
| }; | |
| }, [parametersCombinations]); | |
| } |
…hen-there-is-only-one-category-available # Conflicts: # designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx # docs/Changelog.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx (1)
Line range hint
21-23: Consider extracting Typography styles to a theme or styled componentThe inline styling with variant and title could be moved to a styled component or theme configuration for better maintainability and consistency across the application.
+const CategoryTypography = styled(Typography)({ + fontSize: "1.25rem", +}); -<Typography title={t("panels.scenarioDetails.tooltip.category", "Category")} variant={"body2"}> +<CategoryTypography title={t("panels.scenarioDetails.tooltip.category", "Category")}> {scenario.processCategory} / -</Typography> +</CategoryTypography>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx(1 hunks)docs/Changelog.md(1 hunks)
🔇 Additional comments (2)
designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.tsx (1)
9-13: LGTM! Clean implementation of the category visibility logic
The hook usage and conditional rendering align well with the PR objective of hiding categories when only one is available. The implementation is type-safe and follows React best practices.
docs/Changelog.md (1)
108-108: LGTM! Changelog entry is accurate and well-placed.
The changelog entry correctly describes the changes introduced in PR #7183 to hide categories in various UI elements when only one category is available. The entry is properly placed in the 1.18 version section.
| import { Skeleton, Typography } from "@mui/material"; | ||
| import { Scenario } from "../../Process/types"; | ||
| import { useGetAllCombinations } from "../../useGetAllCombinations"; |
There was a problem hiding this comment.
💡 Codebase verification
Hook requires test coverage for critical functionality
The useGetAllCombinations hook contains important business logic including:
- Fetching scenario parameter combinations
- Managing loading states
- Determining category field visibility
- Handling engine setup errors
Currently, there are no dedicated tests for this hook. Given its critical role in:
- State management
- API integration
- Visibility logic
- Error handling
Test coverage should be added to ensure reliability.
Key areas to test:
- Successful data fetching and state updates
- Loading state transitions
- Category visibility logic
- Error state handling
- Edge cases with empty or error responses
🔗 Analysis chain
Verify test coverage for useGetAllCombinations hook
The component now relies on this hook for critical functionality. Ensure comprehensive test coverage exists for the hook, especially for edge cases around category visibility determination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of the useGetAllCombinations hook
# Look for test files related to the hook
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts useGetAllCombinations
# Search for test cases covering visibility logic
rg -l "describe.*useGetAllCombinations|test.*useGetAllCombinations|it.*useGetAllCombinations"
Length of output: 168
Script:
#!/bin/bash
# Let's try a broader search to locate the hook file first and any potential test files
fd useGetAllCombinations
# Also search for any test files in the same directory as the component
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . designer/client/src/components/
# Look for any test assertions or descriptions containing "combinations" which might be related
rg -l "describe.*combination|test.*combination|it.*combination" designer/client/src/
Length of output: 621
Script:
#!/bin/bash
# Let's examine the hook implementation to understand its complexity and test requirements
cat designer/client/src/components/useGetAllCombinations.ts
# Check if there are any existing tests that might cover this hook indirectly
rg -A 5 -B 5 "useGetAllCombinations" designer/client/src/components/toolbars/scenarioDetails/CategoryDetails.test.tsx
# Look for any integration tests that might cover this functionality
rg -A 5 "CategoryDetails" designer/client/src/components/toolbars/scenarioDetails/__tests__
Length of output: 1949
…hen-there-is-only-one-category-available # Conflicts: # docs/Changelog.md
Dzuming
deleted the
NU-1890-hide-categories-when-there-is-only-one-category-available
branch
Describe your changes
When only one category available:
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
useGetAllCombinations, for improved state management and data fetching in the process dialog components.Bug Fixes
processingModeproperty in the process form.Documentation