fix(ui): BundleAssets/Modules/Packages - do not clear sort params when openning entry side info #4826
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on renaming and restructuring components across several files in the UI package. The property Changes
Possibly related PRs
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 (
|
#11653 Bundle Size — 381.31KiB (~-0.01%).80a7c7c(current) vs a7a7ff2 master#11648(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #11653 |
Baseline #11648 |
|
|---|---|---|
 Initial JS |
334.43KiB(~-0.01%) |
334.44KiB |
 Initial CSS |
46.89KiB |
46.89KiB |
 Cache Invalidation |
28.65% |
28.59% |
| Chunks |
3 |
3 |
| Assets |
4 |
4 |
| Modules |
699 |
699 |
| Duplicate Modules |
0 |
0 |
| Duplicate Code |
0% |
0% |
| Packages |
39 |
39 |
| Duplicate Packages |
1 |
1 |
Bundle size by type 1 change 1 improvement
| Current #11653 |
Baseline #11648 |
|
|---|---|---|
| JS |
334.43KiB (~-0.01%) |
334.44KiB |
| CSS |
46.89KiB |
46.89KiB |
Bundle analysis report Branch fix-entry-link-sort-params Project dashboard
Generated by RelativeCI Documentation Report issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/ui/src/components/asset-name/asset-name.tsx (2)
19-19: LGTM! Consider adding JSDoc for the EntryComponentLink prop.The rename to
EntryComponentLinkimproves semantic clarity. Consider adding JSDoc documentation to describe the expected behavior and usage of this component prop.+ /** Component used to render entry links */ EntryComponentLink: ElementType;
Migration from
customComponentLinktoEntryComponentLinkis incompleteThe codebase shows inconsistent component naming:
- Several files still use the old
customComponentLinkprop/typeEntryComponentLinkis used in some components but not uniformly adoptedKey files needing updates:
packages/ui/src/components/insights/insights.tsxpackages/ui/src/components/module-info/module-info.tsxpackages/ui/src/components/package-info/package-info.tsxpackages/ui/src/components/bundle-packages/bundle-packages.jsxpackages/ui/src/components/bundle-modules/bundle-modules.tsxpackages/ui/src/components/bundle-assets/bundle-assets.jsxpackages/ui/src/components/bundle-assets-totals/bundle-assets-totals.jsxpackages/ui/src/components/asset-info/asset-info.tsx🔗 Analysis chain
Line range hint
38-56: LGTM! Verify consistent usage across the codebase.The EntryComponentLink implementation looks correct. Let's verify that this component naming is used consistently across other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of EntryComponentLink across the codebase # Expected: All occurrences should use the new name, not the old customComponentLink echo "Checking for any remaining instances of old name:" rg "customComponentLink" --type tsx --type jsx echo "Verifying usage of new name:" rg "EntryComponentLink" --type tsx --type jsxLength of output: 324
Script:
#!/bin/bash # Let's try with ripgrep without file type filters since they're not supported echo "Checking for any remaining instances of old name:" rg "customComponentLink" echo -e "\nVerifying usage of new name:" rg "EntryComponentLink"Length of output: 5580
packages/ui/src/components/bundle-assets/bundle-assets.jsx (1)
228-230: Optimize useCallback dependencies.The
renderRowHeaderfunction's dependencies array includesfiltersandsearchwhich aren't used within the callback. These can be safely removed.const renderRowHeader = useCallback( (row) => ( <AssetName row={row} EntryComponentLink={EntryComponentLink} className={css.assetName} /> ), - [EntryComponentLink, filters, search], + [EntryComponentLink], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/ui/src/components/asset-name/asset-name.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.jsx(2 hunks)packages/ui/src/components/bundle-modules/bundle-modules.tsx(4 hunks)packages/ui/src/components/bundle-packages/bundle-packages.jsx(9 hunks)
🔇 Additional comments (13)
packages/ui/src/components/asset-name/asset-name.tsx (1)
23-23: LGTM!
The props destructuring correctly reflects the type changes and maintains clean code structure.
packages/ui/src/components/bundle-assets/bundle-assets.jsx (2)
393-394: LGTM! PropTypes correctly updated.
The sort prop types are properly updated to use field instead of sortBy, maintaining consistency with the component's implementation.
207-223: LGTM! Implementation correctly preserves sort parameters.
The EntryComponentLink implementation now properly includes sort parameters (sortBy and direction) when generating links, which fixes the issue of clearing sort params when opening entry side info. The function is correctly memoized with appropriate dependencies.
Let's verify the component's usage across the codebase:
✅ Verification successful
Sort parameters are correctly preserved across component links
The verification confirms that:
- The
EntryComponentLinkimplementation is used consistently across bundle components (assets, modules, packages). - Sort parameters (
sortByanddirection) are properly passed through the component hierarchy:- Correctly included in component link params
- Properly typed in the codebase
- Consistently handled in the routing logic
- The implementation maintains sort state when navigating between views, which aligns with the PR's objective to fix the issue where sort params were being cleared.
The changes are well-integrated with the existing sorting infrastructure and maintain consistency across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of EntryComponentLink and sort parameters
# Test 1: Check for any remaining instances of old prop name
rg "customComponentLink" --type jsx --type tsx
# Test 2: Verify sort parameters are consistently passed
rg "sortBy|direction" --type jsx --type tsx -C 2
Length of output: 153
Script:
#!/bin/bash
# Let's try without file type filters since jsx/tsx extensions aren't recognized
# First, find relevant files
echo "=== Finding relevant files ==="
fd "\.(jsx?|tsx?)$"
echo -e "\n=== Checking for customComponentLink usage ==="
rg "customComponentLink" -g "*.{js,jsx,ts,tsx}"
echo -e "\n=== Checking sort parameter patterns ==="
rg "sortBy|direction" -g "*.{js,jsx,ts,tsx}" -C 2
echo -e "\n=== Checking EntryComponentLink usage ==="
rg "EntryComponentLink" -g "*.{js,jsx,ts,tsx}" -C 2
Length of output: 52860
packages/ui/src/components/bundle-modules/bundle-modules.tsx (5)
50-61: LGTM! Clean implementation of RowHeader component.
The component correctly implements the renamed prop while maintaining its focused responsibility of rendering the module header.
199-219: LGTM! Successfully preserves sort parameters.
This implementation directly addresses the PR objective by:
- Preserving sort parameters in the URL via the params object
- Maintaining type safety with proper TypeScript usage
- Correctly memoizing the callback with all required dependencies
The change ensures sort parameters aren't cleared when opening entry side info.
234-235: LGTM! Clean renderRowHeader implementation.
The renderRowHeader function is correctly simplified to use the new EntryComponentLink while maintaining proper memoization.
199-219: Verify the impact on sort parameters.
The implementation looks correct, but let's verify that sort parameters are indeed preserved when opening entry side info.
#!/bin/bash
# Search for any other components that might handle sort parameters in a similar way
rg "sortBy.*direction.*entryId" --type tsx --type jsx -B 2 -A 2
# Search for any test files that verify this behavior
rg "test.*sort.*parameters" --type tsx --type jsxAlso applies to: 234-235
46-46: LGTM! Note: Breaking change in prop name.
The renaming from customComponentLink to EntryComponentLink is a good improvement for clarity. However, this is a breaking change that requires updates in all consuming components.
Let's verify all usages of this prop:
✅ Verification successful
Based on the search results, I can now generate the final response:
Breaking change detected, but all usages have been updated
The renaming from customComponentLink to EntryComponentLink is a breaking change, but the search results show that all consuming components have been properly updated to use the new prop name. The changes include:
AssetNamecomponent now usesEntryComponentLinkPackageNameandRowHeadercomponents in bundle-packagesBundleModulescomponent itself
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of customComponentLink in RowHeader
rg "customComponentLink.*RowHeader" --type tsx --type jsx
Length of output: 173
Script:
#!/bin/bash
# Search for any usage of customComponentLink in the codebase
rg "customComponentLink" -A 2 -B 2
# Search for any usage of EntryComponentLink in the codebase
rg "EntryComponentLink" -A 2 -B 2
# Search for files containing BundleModules component usage
rg "BundleModules" -A 2 -B 2
Length of output: 31538
packages/ui/src/components/bundle-packages/bundle-packages.jsx (5)
Line range hint 49-78: LGTM! Clean prop renaming and improved link handling.
The refactoring from customComponentLink to EntryComponentLink improves code clarity. The component now correctly handles the entry ID for linking, which aligns with the PR's objective of maintaining state during navigation.
Line range hint 81-108: LGTM! Consistent prop renaming.
The changes maintain consistency with the PackageName component updates and properly propagate the EntryComponentLink prop.
🧰 Tools
🪛 Biome
[error] 87-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
112-121: LGTM! Clear props organization.
Props destructuring is well-organized and includes all necessary parameters for handling search and entry info display.
402-412: LGTM! Proper prop types definition.
The prop types are correctly updated to reflect the implementation changes, including the sort field renaming and allItems shape definition.
228-245: LGTM! Proper state preservation in link generation.
The implementation correctly preserves all state parameters (filters, search, sort) when generating links, which directly addresses the PR's objective. The sort parameter has been updated to use field instead of sortBy.
Let's verify the sort parameter usage consistency:
✅ Verification successful
Let me analyze the usage of sortBy across the codebase to verify consistency. I'll check the specific implementation in the component links utility since it seems to be the source of truth.
Based on the analysis of the codebase, I can now provide the final response:
The sort parameter usage is consistent and correct
The change from sortBy to sort.field in the link generation is actually implementing the correct pattern. The sortBy parameter in the URL/state management layer (component-links.ts) is properly mapped to the internal sort state object ({ field, direction }) in the UI components. This is evidenced by:
ComponentLinkParamsinterface in component-links.ts definessortByas the URL parameter- UI components receive
sortByas a prop and map it tosort.fieldinternally - The link generation correctly maps back
sort.fieldtosortByfor URL consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining usage of sortBy in sort-related code
rg -g '*.{js,jsx,ts,tsx}' 'sortBy'
Length of output: 2140
Script:
#!/bin/bash
# Check the implementation in component-links.ts
cat packages/utils/src/utils/component-links.ts
Length of output: 15122
…n openning entry side info
vio
force-pushed
the
fix-entry-link-sort-params
branch
from
4d5223e
to
80a7c7c
Compare
Summary by CodeRabbit
New Features
EntryComponentLinkto replaceCustomComponentLink, simplifying component structures across multiple components.BundleAssets.Bug Fixes
RowHeader,PackageName, andBundleModulescomponents for improved clarity and maintainability.Documentation