Assets chunks filter #4843
Assets chunks filter #4843
Conversation
WalkthroughThe changes in this pull request involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant AssetUtils
participant SearchParams
User->>UI: Interact with Bundle Assets
UI->>AssetUtils: Call utility functions (removed)
UI->>SearchParams: Call useSearchParams with filters
SearchParams-->>UI: Return search and filter state
UI-->>User: Display updated Bundle Assets
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 (
|
#11685 Bundle Size — 380.7KiB (-0.16%).f4bd60c(current) vs b24d870 master#11677(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #11685 |
Baseline #11677 |
|
|---|---|---|
 Initial JS |
333.81KiB(-0.19%) |
334.44KiB |
 Initial CSS |
46.89KiB |
46.89KiB |
 Cache Invalidation |
40.94% |
28.65% |
| 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 #11685 |
Baseline #11677 |
|
|---|---|---|
| JS |
333.81KiB (-0.19%) |
334.44KiB |
| CSS |
46.89KiB |
46.89KiB |
Bundle analysis report Branch assets-chunks-filter 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 (5)
packages/ui/src/components/bundle-assets/index.jsx (1)
Line range hint
83-91: Add PropType definition for the chunks propThe
chunksprop is being passed to BundleAssetsComponent but is not defined in the PropTypes. This should be added to maintain proper type checking.Add the following to the PropTypes definition:
BundleAssets.propTypes = { jobs: PropTypes.arrayOf(PropTypes.object).isRequired, filters: PropTypes.object, search: PropTypes.string, sortBy: PropTypes.string, direction: PropTypes.string, setState: PropTypes.func.isRequired, + chunks: PropTypes.arrayOf(PropTypes.shape({ + id: PropTypes.string.isRequired, + name: PropTypes.string.isRequired, + })), }; BundleAssets.defaultProps = { filters: undefined, search: undefined, sortBy: undefined, direction: undefined, + chunks: [], };packages/ui/src/components/bundle-assets/bundle-assets.utils.js (2)
126-140: Consider memoizing the checkedChunkIds array.The current implementation rebuilds the
checkedChunkIdsarray on every filter generation. While the implementation is correct, for better performance, consider memoizing this array when filters change.export const generateGetRowFilter = ({ chunkIds }) => { + let memoizedCheckedChunkIds = null; + let memoizedFilters = null; + + const getCheckedChunkIds = (filters) => { + if (memoizedFilters === filters) { + return memoizedCheckedChunkIds; + } + const checkedChunkIds = []; chunkIds.forEach((chunkId) => { if (filters[`${ASSET_CHUNK}.${chunkId}`]) { checkedChunkIds.push(chunkId); } }); + memoizedCheckedChunkIds = checkedChunkIds; + memoizedFilters = filters; + return checkedChunkIds; + }; return (filters) => { - const checkedChunkIds = []; - chunkIds.forEach((chunkId) => { - if (filters[`${ASSET_CHUNK}.${chunkId}`]) { - checkedChunkIds.push(chunkId); - } - }); + const checkedChunkIds = getCheckedChunkIds(filters);
141-169: Consider optimizing the chunk filtering logic.The implementation is correct, but the intersection operation could be expensive for large datasets. Consider using a Set for O(1) lookups when checking chunk IDs.
if (hasChunkFilters) { const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || []; - return intersection(rowRunsChunkIds, checkedChunkIds).length > 0; + const checkedChunkIdSet = new Set(checkedChunkIds); + return rowRunsChunkIds.some(id => checkedChunkIdSet.has(id)); }packages/ui/src/components/bundle-assets/bundle-assets.jsx (2)
Line range hint
55-111: LGTM: Well-structured chunk filtering implementationThe enhanced getFilters function properly implements chunk filtering while maintaining backward compatibility. The chunks are appropriately sorted by name and have sensible default values.
Consider adding type safety for the chunks parameter:
-const getFilters = ({ compareMode, filters, chunks }) => { +const getFilters = ({ compareMode, filters, chunks = [] }) => {
Line range hint
391-394: Fix PropTypes validation for chunks propThe current PropTypes definition for chunks is incorrect. It should use PropTypes.shape for proper object validation.
- chunks: PropTypes.arrayOf({ - id: PropTypes.string, - name: PropTypes.string, - }).isRequired, + chunks: PropTypes.arrayOf( + PropTypes.shape({ + id: PropTypes.string.isRequired, + name: PropTypes.string.isRequired, + }) + ).isRequired,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/ui/src/components/bundle-assets/bundle-assets.jsx(5 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(2 hunks)packages/ui/src/components/bundle-assets/index.jsx(2 hunks)packages/utils/src/config/component-links.js(1 hunks)packages/utils/src/utils/component-links.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/utils/src/utils/component-links.ts
🔇 Additional comments (9)
packages/utils/src/config/component-links.js (2)
2-2: Verify integration with chunk filtering logic
The new constant is part of enhancing chunk filtering capabilities. Let's verify its integration with the filtering functions mentioned in the summary.
✅ Verification successful
Integration with chunk filtering logic is properly implemented
The ASSET_CHUNK constant is well integrated into the codebase's filtering logic:
- Used in
packages/ui/src/components/bundle-assets/bundle-assets.jsxfor chunk filtering UI and state management - Integrated in
packages/ui/src/components/bundle-assets/bundle-assets.utils.jsfor filtering assets based on chunk IDs - Part of the filtering system that allows users to filter assets by chunk type and specific chunks
The constant is used consistently with the existing filtering patterns and properly supports the chunk filtering capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration with getFilters and generateGetRowFilter
# Expected: Find the filtering logic implementation to ensure proper usage of ASSET_CHUNK
echo "=== Searching for getFilters implementation ==="
ast-grep --pattern 'function getFilters($$$) { $$$ }'
echo -e "\n=== Searching for generateGetRowFilter implementation ==="
ast-grep --pattern 'function generateGetRowFilter($$$) { $$$ }'
Length of output: 452
Script:
#!/bin/bash
# Let's try a different approach to find the filtering logic and usage of ASSET_CHUNK
echo "=== Searching for files containing ASSET_CHUNK ==="
rg "ASSET_CHUNK" -A 5
echo -e "\n=== Searching for filter-related functions ==="
rg -i "filter.*chunk" -A 5
Length of output: 20343
2-2:
Consider using distinct identifiers for ASSET_CHUNK and MODULE_CHUNK
Both ASSET_CHUNK and MODULE_CHUNK use the same identifier 'c' which could lead to confusion in filtering logic. Consider using distinct identifiers to prevent potential bugs.
Let's verify how these constants are used across the codebase:
packages/ui/src/components/bundle-assets/index.jsx (3)
25-25: LGTM! Clean implementation of chunks data extraction
The memoization is properly implemented with correct dependencies, and the destructuring pattern is clean.
60-61: Verify edge cases in the new filter generator
The change to a generator pattern is good, but we should ensure it handles edge cases properly:
- Empty chunk IDs array
- Invalid chunk IDs
- Null/undefined values
#!/bin/bash
# Description: Look for edge case handling in generateGetRowFilter
# Search for null checks or validation in the filter generator
ast-grep --pattern 'function generateGetRowFilter($_) {
$$$
if ($_ === null || $_ === undefined) {
$$$
}
$$$
}'
# Look for test cases covering edge cases
rg -A 5 "describe.*generateGetRowFilter" --type js16-20: Verify usage of imported utilities
The imports look well-structured, but let's verify that all imported utilities are being used effectively in the codebase.
✅ Verification successful
All imported utilities are properly used in the codebase
Based on the search results, all three imported utilities are actively used in the component:
addMetricReportAssetRowDatais used in the rows calculation:webpack.compareBySection.assets(jobs, [addMetricReportAssetRowData])generateGetRowFilteris used in the filtered rows logic:getRowFilter: generateGetRowFilter({ chunkIds })getCustomSortis used in the sort parameters:getCustomSortis passed touseRowsSort
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of imported utilities from bundle-assets.utils
# Check for usage of addMetricReportAssetRowData
rg "addMetricReportAssetRowData" --type js
# Check for usage of generateGetRowFilter
rg "generateGetRowFilter" --type js
# Check for usage of getCustomSort
rg "getCustomSort" --type js
Length of output: 178
Script:
#!/bin/bash
# Let's try a more targeted search within the bundle-assets directory
# First, let's locate the bundle-assets directory
fd -t d "bundle-assets"
# Then search for the utility functions in the component and utils files
rg "addMetricReportAssetRowData|generateGetRowFilter|getCustomSort" -l
# Let's also check the actual implementation file
rg "addMetricReportAssetRowData|generateGetRowFilter|getCustomSort" -A 5 -B 5
# Check for any test files that might use these functions
fd -e test.js -e test.jsx -e spec.js -e spec.jsx | xargs rg "addMetricReportAssetRowData|generateGetRowFilter|getCustomSort"
Length of output: 22472
packages/ui/src/components/bundle-assets/bundle-assets.utils.js (1)
7-14: LGTM! Import changes align with new chunk filtering functionality.
The addition of intersection from lodash and ASSET_CHUNK constant is appropriate for implementing the new chunk filtering capabilities.
packages/ui/src/components/bundle-assets/bundle-assets.jsx (3)
5-6: LGTM: Import additions are appropriate
The new lodash utility imports and ASSET_CHUNK constant are properly added to support the chunk filtering functionality.
Also applies to: 9-9
214-217: LGTM: Improved variable naming and proper dependency tracking
The rename from dropdownFilters to filterFieldsData better reflects the variable's purpose. The dependencies array correctly includes the chunks parameter.
64-77: Verify chunk filtering behavior
Let's verify the following aspects of chunk filtering:
- Performance with large number of chunks
- Filter state persistence
- URL parameter handling
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Chunk filtering implementation is well-optimized and safe
The implementation shows good practices for chunk filtering:
- Chunks are memoized at the component level using
useMemo - Performance is optimized with
orderByfor consistent sorting - Filter state is properly managed through props and default values
- URL parameters are handled through the standard filter management system
The changes introduce no performance concerns as:
- Chunk filtering is applied only when chunks exist (
!isEmpty(chunks)) - Sorting is done once during filter creation
- Default values are safely handled with nullish coalescing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance issues with chunk filtering
# Check for chunk-related URL parameter handling
rg -A 5 "ASSET_CHUNK" --type js
# Look for filter state persistence implementation
rg -A 10 "updateFilters|resetFilters" --type js
# Check for existing performance optimizations
rg -A 5 "useMemo.*chunks" --type js
Length of output: 14405
Script:
#!/bin/bash
# Let's check for performance optimizations and memoization patterns
# Check for useMemo with chunks in bundle-assets component
ast-grep --pattern 'useMemo(() => {
$$$
}, [$$$chunks$$$])'
# Check for chunk filtering implementation
rg -A 10 "filterAssets" --type js
# Check for URL parameter handling related to chunks
rg -A 10 "searchParams.*chunk" --type js
Length of output: 140
Script:
#!/bin/bash
# Let's check the bundle-assets component implementation more thoroughly
# Check for chunk-related state management
rg -A 10 "const.*chunks.*=" packages/ui/src/components/bundle-assets/
# Look for performance-related implementations
rg -A 5 "orderBy.*chunks" packages/ui/src/components/bundle-assets/
# Check filter-related utilities
rg -A 10 "getFilteredAssets|filterItems" packages/ui/src/components/bundle-assets/
Length of output: 3646
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
packages/ui/src/types.d.ts (2)
15-17: Well-structured type definition!The new interface properly extends
FilterFieldDataand makes thekeyrequirement explicit, which improves type safety and maintainability.Consider documenting the purpose of the
keyfield with JSDoc comments, especially if it needs to be unique or follow specific patterns.
21-21: Excellent type refactoring!The change to use
Array<ChildFilterFieldData>improves code maintainability by centralizing the type definition and reducing duplication.Consider adding a type alias for
Array<ChildFilterFieldData>if this type combination is used in multiple places, e.g.:type FilterChildren = Array<ChildFilterFieldData>;packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (2)
25-31: Consider adding component documentationThe meta configuration is well-structured and follows Storybook best practices. Consider enhancing it with:
- A description of the component's purpose
- Parameters for viewport/backgrounds if relevant
- Documentation about the setState action's expected behavior
const meta: Meta<typeof BundleAssets> = { title: 'Components/BundleAssets', component: BundleAssets, decorators: [getWrapperDecorator()], + parameters: { + docs: { + description: { + component: 'BundleAssets component displays...', + }, + }, + }, args: { setState: action('STATE'), }, };
Line range hint
38-99: Consider enhancing stories organization and documentationThe stories are well-structured and properly typed. Consider these improvements:
- Group related stories together (e.g., move EmptyFilteredData next to NoAssets)
- Document each story's test scenario
- Extract complex data transformations to utility functions
Example enhancement:
export const NoAssets: Story = { + parameters: { + docs: { + description: 'Displays component behavior when no assets are present', + }, + }, args: { jobs: JOBS.map((job) => set(merge({}, job), 'metrics.webpack.assets', {})), }, };Consider extracting data transformations:
// utils.ts export const createEmptyAssetsJob = (job: Job) => set(merge({}, job), 'metrics.webpack.assets', {}); // stories.tsx export const NoAssets: Story = { args: { jobs: JOBS.map(createEmptyAssetsJob), }, };packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (5)
28-51: Suggestion: UseArray.prototype.some()for readabilityIn the
getIsNotPredictivefunction, consider usingArray.prototype.some()instead ofreduce()to improve readability, as you're checking if any run meets a certain condition.-export const getIsNotPredictive = (row: ReportMetricRow): boolean => { - const { key, runs } = row; - - return runs.reduce((agg, run, index) => { - if (agg) { - return agg; - } - if (index + 1 === runs.length) { - return agg; - } - const preRun = runs[index + 1]; - if ( - run && - preRun && - key !== run.name && - run.name === preRun.name && - run.value !== preRun.value - ) { - return true; - } - return agg; - }, false); -}; +export const getIsNotPredictive = (row: ReportMetricRow): boolean => { + const { key, runs } = row; + return runs.some((run, index) => { + if (index + 1 === runs.length) { + return false; + } + const preRun = runs[index + 1]; + return ( + run && + preRun && + key !== run.name && + run.name === preRun.name && + run.value !== preRun.value + ); + }); +};
59-64: Simplify undefined value filteringIn
getAssetMetaStatus, you can simplify the filtering of undefined values by usingvalue !== undefinedinstead oftypeof value !== 'undefined'.-const metaValues = values.filter((value) => typeof value !== 'undefined'); +const metaValues = values.filter((value) => value !== undefined);
217-217: Remove unnecessary ESLint disable commentThe
/* eslint-disable prettier/prettier */comment seems unnecessary as the code follows Prettier formatting. Removing it helps keep the code clean.-/* eslint-disable prettier/prettier */
221-227: Usefilterinstead offorEachfor collectingcheckedChunkIdsIn
generateGetRowFilter, consider usingfilterto simplify the code when collectingcheckedChunkIds.-const checkedChunkIds: Array<string> = []; - -chunkIds.forEach((chunkId) => { - if (filters[`${ASSET_CHUNK}.${chunkId}`]) { - checkedChunkIds.push(chunkId); - } -}); +const checkedChunkIds = chunkIds.filter((chunkId) => filters[`${ASSET_CHUNK}.${chunkId}`]);
237-246: Simplify type filter conditions for better readabilityThe condition in
generateGetRowFilterchecking the type filters can be simplified to improve readability.-if ( - !( - (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) || - (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) || - (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) || - (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset) - ) -) { - return false; -} +const typeMatches = + (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) || + (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) || + (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) || + (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset); +if (!typeMatches) { + return false; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/ui/src/components/bundle-assets/bundle-assets.jsx(5 hunks)packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.ts(1 hunks)packages/ui/src/types.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/ui/src/components/bundle-assets/bundle-assets.utils.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/bundle-assets/bundle-assets.jsx
🔇 Additional comments (3)
packages/ui/src/types.d.ts (1)
15-21: Verify type compatibility with existing code
These type changes might affect existing code that uses FilterGroupFieldData.
✅ Verification successful
Let me try a different search strategy to verify the type changes.
Based on the search results, I can now provide a final response:
Type changes appear to be safe and consistent with existing usage
The search results show that the FilterGroupFieldData and ChildFilterFieldData types are used consistently across the codebase:
- The types are used correctly in filters.tsx, filters.utils.ts, and bundle-modules.utils.ts
- All usages follow the same structure where
FilterGroupFieldDatacontainschildrenas an array ofChildFilterFieldData - No type assertions or incompatible usages were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of FilterGroupFieldData to ensure compatibility
echo "Searching for FilterGroupFieldData usage patterns..."
rg --type typescript "FilterGroupFieldData" -B 2 -A 2
# Search for potential type assertions or casts that might break
echo "Checking for type assertions that might need updates..."
ast-grep --pattern 'as FilterGroupFieldData'
Length of output: 371
Script:
#!/bin/bash
# Search for all TypeScript/JavaScript files that might use these types
echo "Searching for files containing FilterGroupFieldData..."
rg "FilterGroupFieldData" -l
# Search for actual usage patterns
echo "Checking usage patterns..."
rg "FilterGroupFieldData" -A 3 -B 3
# Look for related type ChildFilterFieldData
echo "Searching for ChildFilterFieldData usage..."
rg "ChildFilterFieldData" -A 3 -B 3
# Check for any .tsx files that might use these types
echo "Checking TSX files specifically..."
fd -e tsx -x rg "FilterGroupFieldData|ChildFilterFieldData" {} -A 3 -B 3
Length of output: 10243
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (2)
11-12: LGTM! Proper TypeScript integration
The addition of proper type imports from Storybook shows good TypeScript integration.
36-36: LGTM! Proper story type definition
The Story type is correctly defined using StoryObj with proper component type inference.
vio
force-pushed
the
assets-chunks-filter
branch
from
13c7de0
to
e3fc3ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (1)
68-68: Consider simplifying data manipulationThe current approach uses nested lodash operations (merge and set) which could be simplified for better readability.
Consider using object spread and direct property assignment:
- jobs: JOBS.map((job) => set(merge({}, job), 'metrics.webpack.assets', {})), + jobs: JOBS.map((job) => ({ + ...job, + metrics: { + ...job.metrics, + webpack: { + ...job.metrics.webpack, + assets: {}, + }, + }, + })),Also applies to: 74-77, 84-96
packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (4)
22-51: Consider improving readability and documentationWhile the function is type-safe, there are several opportunities for improvement:
- The nested conditions make the logic hard to follow
- The variable name
aggcould be more descriptive (e.g.,isNotPredictive)- The documentation could better explain what makes an asset cache "not predictive"
Consider this refactoring:
/** * Check if the asset cache is not predictive + * + * An asset is considered not predictive when: + * - It appears in consecutive runs + * - Has the same name but different values + * - The key doesn't match the name */ export const getIsNotPredictive = (row: ReportMetricRow): boolean => { const { key, runs } = row; - return runs.reduce((agg, run, index) => { - if (agg) { - return agg; + return runs.reduce((isNotPredictive, run, index) => { + if (isNotPredictive) { + return isNotPredictive; } if (index + 1 === runs.length) { - return agg; + return isNotPredictive; } const preRun = runs[index + 1]; + const hasSameNameDifferentValue = + run?.name === preRun?.name && run?.value !== preRun?.value; - if ( - run && - preRun && - key !== run.name && - run.name === preRun.name && - run.value !== preRun.value - ) { + if (run && preRun && key !== run.name && hasSameNameDifferentValue) { return true; } - return agg; + return isNotPredictive; }, false); };
53-84: Enhance type safety and documentationThe function logic is sound, but could benefit from improved type safety and documentation.
Consider these improvements:
+type AssetStatusValue = boolean | undefined; + /** * Return asset meta status + * + * @example + * getAssetMetaStatus([true]) // true + * getAssetMetaStatus([true, false]) // 'added' + * getAssetMetaStatus([false, true]) // 'removed' + * getAssetMetaStatus([false, false]) // false */ export const getAssetMetaStatus = ( - values: Array<boolean>, + values: Array<AssetStatusValue>, ): ReportMetricAssetRowMetaStatus | boolean => { if (!values.includes(true)) { return false; } // filter empty runs - const metaValues = values.filter((value) => typeof value !== 'undefined'); + const definedValues = values.filter((value): value is boolean => + typeof value !== 'undefined' + ); - const current = metaValues[0]; - const metaValuesLength = metaValues.length; + const currentValue = definedValues[0]; + const totalValues = definedValues.length; - if (metaValuesLength === 1) { - return Boolean(current); + if (totalValues === 1) { + return Boolean(currentValue); } - const baseline = metaValues[metaValuesLength - 1]; + const baselineValue = definedValues[totalValues - 1]; - if (current && !baseline) { + if (currentValue && !baselineValue) { return 'added'; } - if (!current && baseline) { + if (!currentValue && baselineValue) { return 'removed'; } return true; };
129-140: Add documentation for sorting criteriaThe sorting logic is not immediately obvious due to the boolean negations.
Consider adding documentation to explain the sorting priority:
+/** + * Generates sorting criteria for asset rows with the following priority: + * 1. Predictive assets first + * 2. Unchanged assets first + * 3. Initial assets first + * 4. Entry assets first + * 5. Chunk assets first + * 6. Alphabetically by key + */ export const getCustomSort = (item: ReportMetricAssetRow): Array<boolean | string> => [ !item.isNotPredictive, // false (predictive) comes first !item.changed, // false (unchanged) comes first !item.isInitial, // false (initial) comes first !item.isEntry, // false (entry) comes first !item.isChunk, // false (chunk) comes first item.key, // alphabetical order ];
142-215: Improve filter configuration type safetyThe filter configuration could benefit from stricter typing and centralized defaults.
Consider these improvements:
+interface FilterDefaults { + readonly [key: string]: { + readonly label: string; + readonly defaultValue: boolean; + }; +} + +const FILTER_DEFAULTS: FilterDefaults = { + [ASSET_FILTERS.ENTRY]: { label: 'Entry', defaultValue: true }, + [ASSET_FILTERS.INITIAL]: { label: 'Initial', defaultValue: true }, + [ASSET_FILTERS.CHUNK]: { label: 'Chunk', defaultValue: true }, + [ASSET_FILTERS.OTHER]: { label: 'Other', defaultValue: true }, +}; + export const getFilters = ({ compareMode, filters, chunks, }: GetFiltersOptions): FilterFieldsData => { const result: FilterFieldsData = { [ASSET_FILTERS.CHANGED]: { label: 'Changed', defaultValue: filters[ASSET_FILTERS.CHANGED], disabled: !compareMode, }, }; if (!isEmpty(chunks)) { const chunksFilter: FilterGroupFieldData = { label: 'Chunks', children: [] }; - const chunksOrderedByName = orderBy(chunks, 'name'); + const chunksOrderedByName = orderBy(chunks, ['name'], ['asc']); chunksOrderedByName.forEach((chunk) => { chunksFilter.children.push({ key: chunk.id, label: chunk.name, defaultValue: filters[`${ASSET_CHUNK}.${chunk.id}`] ?? true, }); }); result[ASSET_CHUNK] = chunksFilter; } result[ASSET_ENTRY_TYPE] = { label: 'Type', - children: [ - { - key: ASSET_FILTERS.ENTRY, - label: 'Entry', - defaultValue: get(filters, `${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`, true), - }, - // ... similar entries - ], + children: Object.entries(FILTER_DEFAULTS).map(([key, { label, defaultValue }]) => ({ + key, + label, + defaultValue: get(filters, `${ASSET_ENTRY_TYPE}.${key}`, defaultValue), + })), };packages/ui/src/components/bundle-assets/bundle-assets.tsx (2)
Line range hint
73-86: Ensure consistent use of 'escapeRegExp' in 'onGroupClick'In the
onGroupClickfunction,escapeRegExpis used when definingnewSearch, but it's not used in the finalupdateSearchcall. This could lead to regex errors ifgroupPathcontains special regex characters.Apply this diff to consistently escape
groupPath:if (newSearch === search) { updateSearch(''); return; } - updateSearch(`^${groupPath}/`); + updateSearch(`^${escapeRegExp(groupPath)}/`);
117-117: Specify a concrete type for 'newFilters' in 'updateFilters'The
updateFiltersfunction acceptsnewFilterstyped asany, which diminishes type safety. Defining a specific type fornewFilterswill enhance type checking and maintainability.Example:
interface Filters { // Define the structure of the filters object } updateFilters: (newFilters: Filters) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/ui/src/components/asset-info/asset-info.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.tsx(10 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.ts(1 hunks)packages/ui/src/types.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/ui/src/components/bundle-assets/bundle-assets.utils.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/types.d.ts
🔇 Additional comments (9)
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (4)
11-12: LGTM: Proper TypeScript integration
The addition of Storybook types and actions imports follows best practices for TypeScript integration.
25-31: LGTM: Well-structured meta configuration
The meta configuration follows Storybook best practices with proper typing and default args setup. Moving setState to default args reduces duplication across stories.
36-36: LGTM: Proper story type definition
The Story type is correctly defined using StoryObj, ensuring proper typing for all stories.
Line range hint 38-99: LGTM: Comprehensive story coverage
The stories provide excellent coverage of various scenarios including default behavior, multiple jobs, custom filters, and edge cases. The consistent structure and proper typing enhance maintainability.
packages/ui/src/components/asset-info/asset-info.tsx (3)
5-5: LGTM: Import of WebpackChunk type
The import of WebpackChunk type aligns with the type system migration.
76-77: Verify ReportMetricAssetRow type compatibility
The type changes look good, but we should verify that ReportMetricAssetRow includes all the required properties used in the component (runs, isEntry, isInitial, isChunk, fileType).
#!/bin/bash
# Search for ReportMetricAssetRow type definition
ast-grep --pattern 'type ReportMetricAssetRow = {
$$$
}'
# Verify all required properties are used consistently
rg -t typescript 'ReportMetricAssetRow.*\{.*\}'23-23: Verify WebpackChunk type compatibility
The change from MetaChunk[] to WebpackChunk[] looks good, but let's verify that the WebpackChunk type maintains the same interface for the id property used in the component.
✅ Verification successful
Based on the code analysis, I can now provide a definitive response about the WebpackChunk type compatibility:
The type change is safe and compatible
The codebase shows that:
- The WebpackChunk type is imported from '@bundle-stats/utils' and is used consistently throughout the codebase
- The ChunkModulesLink component only uses the
idproperty from chunks, which is defined as a string in both MetaChunk and WebpackChunk interfaces - The usage pattern in the component matches the type definition, where chunks.find() looks up chunks by id property
- The type change is part of a broader refactoring where WebpackChunk is the standardized type across the codebase
The change from MetaChunk[] to WebpackChunk[] maintains interface compatibility for the id property that the component depends on.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WebpackChunk type definition to verify the id property
ast-grep --pattern 'type WebpackChunk = {
$$$
id: $_
$$$
}'
# Search for any usage of chunks.id to ensure compatibility
rg -t typescript -t javascript 'chunks.*\.id'
Length of output: 244
Script:
#!/bin/bash
# Search for WebpackChunk type/interface definition
rg -t ts -t tsx -t js -t jsx "type WebpackChunk|interface WebpackChunk"
# Search for usage of chunks.id in the component
fd -e tsx -e ts "asset-info" --exec cat {}
# Search for MetaChunk type/interface for comparison
rg -t ts -t tsx -t js -t jsx "type MetaChunk|interface MetaChunk"
Length of output: 6571
Script:
#!/bin/bash
# Try different command to search for type definitions
fd -e ts -e tsx . --exec grep -l "WebpackChunk\|MetaChunk" {}
# Look specifically in utils package for type definitions
fd . "packages/utils" -e ts -e tsx --exec cat {}
# Search for chunk usage in the component
rg --type-add 'ts:*.{ts,tsx}' -t ts 'chunks?.find\(\({ id }\)'
Length of output: 141904
Script:
#!/bin/bash
# Search for WebpackChunk interface/type definition
rg -t ts -t tsx "interface WebpackChunk|type WebpackChunk" --no-filename
# Search for MetaChunk interface/type definition for comparison
rg -t ts -t tsx "interface MetaChunk|type MetaChunk" --no-filename
# Look for usage of chunks.id in the component file
fd "asset-info.tsx" -x cat {}
Length of output: 5075
Script:
#!/bin/bash
# Look for WebpackChunk type definition in the utils package
fd . "packages/utils" -t f -e ts -e tsx -x grep -l "WebpackChunk"
# Try to find the actual type definition in the found files
fd . "packages/utils" -t f -e ts -e tsx -x cat {}
Length of output: 141271
packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (2)
1-21: LGTM: Well-structured imports with proper type definitions
The imports are well-organized, using specific lodash functions instead of the entire library, and properly importing types from @bundle-stats/utils.
217-260: 🛠️ Refactor suggestion
Optimize filter generation and improve readability
The filter generation logic could be simplified and optimized.
Consider these improvements:
-/* eslint-disable prettier/prettier */
+interface RowFilters {
+ [key: string]: unknown;
+}
+
+type RowFilterFn = (item: ReportMetricAssetRow) => boolean;
+
export const generateGetRowFilter =
- ({ chunkIds }: GenerateGetRowFilterOptions) => (filters: Record<string, unknown>) => {
+ ({ chunkIds }: GenerateGetRowFilterOptions) => (filters: RowFilters): RowFilterFn => {
// List of chunkIds with filter value set to `true`
- const checkedChunkIds: Array<string> = [];
-
- chunkIds.forEach((chunkId) => {
- if (filters[`${ASSET_CHUNK}.${chunkId}`]) {
- checkedChunkIds.push(chunkId);
- }
- });
+ const checkedChunkIds = chunkIds.filter(
+ (chunkId) => filters[`${ASSET_CHUNK}.${chunkId}`]
+ );
const hasChunkFilters =
checkedChunkIds.length > 0 && checkedChunkIds.length !== chunkIds.length;
+ const isTypeMatched = (item: ReportMetricAssetRow): boolean => (
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset)
+ );
+
return (item: ReportMetricAssetRow) => {
+ // Early returns for simple conditions
if (filters[ASSET_FILTERS.CHANGED] && !item.changed) {
return false;
}
- if (
- !(
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset)
- )
- ) {
+ if (!isTypeMatched(item)) {
return false;
}
if (!filters[`${ASSET_FILE_TYPE}.${item.fileType}`]) {
return false;
}
// Filter if any of the chunkIds are checked
if (hasChunkFilters) {
- const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || [];
+ const rowRunsChunkIds = item.runs?.map((run) => run?.chunkId).filter(Boolean) ?? [];
return intersection(rowRunsChunkIds, checkedChunkIds).length > 0;
}
return true;
};
};These changes:
- Add proper TypeScript interfaces
- Simplify chunk ID filtering using array methods
- Extract type matching logic into a separate function
- Improve null handling in chunk ID extraction
- Remove unnecessary eslint disable comment
Let's verify the impact of these changes:
✅ Verification successful
Let's gather more information about the usage and dependencies of this function.
Let's check the implementation of similar filter generators to ensure our suggested changes align with the codebase patterns.
Based on the analysis of similar implementations and usage patterns across the codebase, I can now provide the final response:
Consider simpler improvements while maintaining consistency
The suggested refactoring should be scaled back to maintain consistency with similar implementations in the codebase, particularly with bundle-modules.utils.ts which has a very similar pattern.
-/* eslint-disable prettier/prettier */
export const generateGetRowFilter =
({ chunkIds }: GenerateGetRowFilterOptions) => (filters: Record<string, unknown>) => {
// List of chunkIds with filter value set to `true`
- const checkedChunkIds: Array<string> = [];
-
- chunkIds.forEach((chunkId) => {
- if (filters[`${ASSET_CHUNK}.${chunkId}`]) {
- checkedChunkIds.push(chunkId);
- }
- });
+ const checkedChunkIds = chunkIds.filter(
+ (chunkId) => filters[`${ASSET_CHUNK}.${chunkId}`]
+ );
const hasChunkFilters =
checkedChunkIds.length > 0 && checkedChunkIds.length !== chunkIds.length;
return (item: ReportMetricAssetRow) => {
if (filters[ASSET_FILTERS.CHANGED] && !item.changed) {
return false;
}
if (
!(
(filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) ||
(filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) ||
(filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) ||
(filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset)
)
) {
return false;
}
if (!filters[`${ASSET_FILE_TYPE}.${item.fileType}`]) {
return false;
}
if (hasChunkFilters) {
- const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || [];
+ const rowRunsChunkIds = item.runs?.map((run) => run?.chunkId).filter(Boolean) ?? [];
return intersection(rowRunsChunkIds, checkedChunkIds).length > 0;
}
return true;
};
};The revised changes:
- Simplify chunk ID filtering using array methods
- Improve null handling in chunk ID extraction
- Remove unnecessary eslint disable comment
- Keep the existing structure to maintain consistency with similar implementations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar filter patterns in the codebase
rg -l 'generateGetRowFilter|getRowFilter' .
Length of output: 450
Script:
#!/bin/bash
# Check the implementation and usage of getRowFilter in related files
echo "=== Bundle Assets Usage ==="
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/components/bundle-assets/index.jsx
echo "\n=== Bundle Modules Usage ==="
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/components/bundle-modules/bundle-modules.utils.ts
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/components/bundle-modules/index.tsx
echo "\n=== Bundle Packages Usage ==="
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/components/bundle-packages/bundle-packages.utils.js
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/components/bundle-packages/index.jsx
echo "\n=== Hooks Usage ==="
rg -A 5 -B 5 'generateGetRowFilter|getRowFilter' ./packages/ui/src/hooks/rows-filter.js
Length of output: 3985
Script:
#!/bin/bash
# Check the implementation of similar filter generators
echo "=== Bundle Modules Filter Implementation ==="
ast-grep --pattern 'export const generateGetRowFilter = $_' ./packages/ui/src/components/bundle-modules/bundle-modules.utils.ts -A 30
echo "=== Bundle Packages Filter Implementation ==="
ast-grep --pattern 'export const getRowFilter = $_' ./packages/ui/src/components/bundle-packages/bundle-packages.utils.js -A 15
# Check for TypeScript interfaces
echo "=== Related TypeScript Interfaces ==="
rg -A 5 'interface.*Row' ./packages/ui/src/components/bundle-assets/
rg -A 5 'interface.*Row' ./packages/ui/src/components/bundle-modules/
Length of output: 11213
| /** | ||
| * Add asset row flags | ||
| */ | ||
| export const addMetricReportAssetRowData = (row: ReportMetricRow): ReportMetricAssetRow => { | ||
| const { changed, runs } = row; | ||
|
|
||
| // Collect meta for each run | ||
| const runsEntry: Array<boolean> = []; | ||
| const runsInitial: Array<boolean> = []; | ||
| const runsChunk: Array<boolean> = []; | ||
|
|
||
| (runs as Array<AssetMetricRun | null>).forEach((run) => { | ||
| runsEntry.push(run?.isEntry || false); | ||
| runsInitial.push(run?.isInitial || false); | ||
| runsChunk.push(run?.isChunk || false); | ||
| }); | ||
|
|
||
| const isEntry = getAssetMetaStatus(runsEntry); | ||
| const isInitial = getAssetMetaStatus(runsInitial); | ||
| const isChunk = getAssetMetaStatus(runsChunk); | ||
| const isAsset = !(isEntry || isInitial || isChunk); | ||
| const isNotPredictive = getIsNotPredictive(row); | ||
| const fileType = getFileType(row.key); | ||
|
|
||
| // Flag asset as changed if name and value are identical, if one of the meta tags is changed | ||
| const assetChanged = | ||
| changed || | ||
| typeof isEntry !== 'boolean' || | ||
| typeof isInitial !== 'boolean' || | ||
| typeof isChunk !== 'boolean'; | ||
|
|
||
| return { | ||
| ...row, | ||
| changed: assetChanged, | ||
| isEntry, | ||
| isInitial, | ||
| isChunk, | ||
| isAsset, | ||
| isNotPredictive, | ||
| fileType, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and reduce complexity
The function has several areas that could be improved for better type safety and maintainability:
- The type casting of runs array (
as Array<AssetMetricRun | null>) could be dangerous - The multiple boolean arrays could be consolidated into a single structure
- The condition for
assetChangedcould be simplified
Consider this refactoring:
+interface AssetRunFlags {
+ isEntry: boolean;
+ isInitial: boolean;
+ isChunk: boolean;
+}
+
export const addMetricReportAssetRowData = (row: ReportMetricRow): ReportMetricAssetRow => {
const { changed, runs } = row;
- // Collect meta for each run
- const runsEntry: Array<boolean> = [];
- const runsInitial: Array<boolean> = [];
- const runsChunk: Array<boolean> = [];
+ // Collect flags for each run
+ const runFlags = (runs as Array<AssetMetricRun | null>).map((run): AssetRunFlags => ({
+ isEntry: run?.isEntry || false,
+ isInitial: run?.isInitial || false,
+ isChunk: run?.isChunk || false,
+ }));
- (runs as Array<AssetMetricRun | null>).forEach((run) => {
- runsEntry.push(run?.isEntry || false);
- runsInitial.push(run?.isInitial || false);
- runsChunk.push(run?.isChunk || false);
- });
+ const isEntry = getAssetMetaStatus(runFlags.map(f => f.isEntry));
+ const isInitial = getAssetMetaStatus(runFlags.map(f => f.isInitial));
+ const isChunk = getAssetMetaStatus(runFlags.map(f => f.isChunk));
- const isEntry = getAssetMetaStatus(runsEntry);
- const isInitial = getAssetMetaStatus(runsInitial);
- const isChunk = getAssetMetaStatus(runsChunk);
const isAsset = !(isEntry || isInitial || isChunk);
const isNotPredictive = getIsNotPredictive(row);
const fileType = getFileType(row.key);
- // Flag asset as changed if name and value are identical, if one of the meta tags is changed
- const assetChanged =
- changed ||
- typeof isEntry !== 'boolean' ||
- typeof isInitial !== 'boolean' ||
- typeof isChunk !== 'boolean';
+ const hasMetaChanges = [isEntry, isInitial, isChunk].some(
+ status => typeof status !== 'boolean'
+ );
+ const assetChanged = changed || hasMetaChanges;📝 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.
| /** | |
| * Add asset row flags | |
| */ | |
| export const addMetricReportAssetRowData = (row: ReportMetricRow): ReportMetricAssetRow => { | |
| const { changed, runs } = row; | |
| // Collect meta for each run | |
| const runsEntry: Array<boolean> = []; | |
| const runsInitial: Array<boolean> = []; | |
| const runsChunk: Array<boolean> = []; | |
| (runs as Array<AssetMetricRun | null>).forEach((run) => { | |
| runsEntry.push(run?.isEntry || false); | |
| runsInitial.push(run?.isInitial || false); | |
| runsChunk.push(run?.isChunk || false); | |
| }); | |
| const isEntry = getAssetMetaStatus(runsEntry); | |
| const isInitial = getAssetMetaStatus(runsInitial); | |
| const isChunk = getAssetMetaStatus(runsChunk); | |
| const isAsset = !(isEntry || isInitial || isChunk); | |
| const isNotPredictive = getIsNotPredictive(row); | |
| const fileType = getFileType(row.key); | |
| // Flag asset as changed if name and value are identical, if one of the meta tags is changed | |
| const assetChanged = | |
| changed || | |
| typeof isEntry !== 'boolean' || | |
| typeof isInitial !== 'boolean' || | |
| typeof isChunk !== 'boolean'; | |
| return { | |
| ...row, | |
| changed: assetChanged, | |
| isEntry, | |
| isInitial, | |
| isChunk, | |
| isAsset, | |
| isNotPredictive, | |
| fileType, | |
| }; | |
| }; | |
| /** | |
| * Add asset row flags | |
| */ | |
| interface AssetRunFlags { | |
| isEntry: boolean; | |
| isInitial: boolean; | |
| isChunk: boolean; | |
| } | |
| export const addMetricReportAssetRowData = (row: ReportMetricRow): ReportMetricAssetRow => { | |
| const { changed, runs } = row; | |
| // Collect flags for each run | |
| const runFlags = (runs as Array<AssetMetricRun | null>).map((run): AssetRunFlags => ({ | |
| isEntry: run?.isEntry || false, | |
| isInitial: run?.isInitial || false, | |
| isChunk: run?.isChunk || false, | |
| })); | |
| const isEntry = getAssetMetaStatus(runFlags.map(f => f.isEntry)); | |
| const isInitial = getAssetMetaStatus(runFlags.map(f => f.isInitial)); | |
| const isChunk = getAssetMetaStatus(runFlags.map(f => f.isChunk)); | |
| const isAsset = !(isEntry || isInitial || isChunk); | |
| const isNotPredictive = getIsNotPredictive(row); | |
| const fileType = getFileType(row.key); | |
| const hasMetaChanges = [isEntry, isInitial, isChunk].some( | |
| status => typeof status !== 'boolean' | |
| ); | |
| const assetChanged = changed || hasMetaChanges; | |
| return { | |
| ...row, | |
| changed: assetChanged, | |
| isEntry, | |
| isInitial, | |
| isChunk, | |
| isAsset, | |
| isNotPredictive, | |
| fileType, | |
| }; | |
| }; |
| }); | ||
| interface ViewMetricsTreemapProps { | ||
| metricsTableTitle: ReactNode; | ||
| jobs: Array<any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'Array' with specific types for 'jobs'
The jobs prop is currently typed as Array<any>, which reduces type safety. Consider defining a specific Job type to enhance type checking and prevent potential runtime errors.
Example:
interface Job {
label: string;
// Include other properties of the job object
}
interface ViewMetricsTreemapProps {
metricsTableTitle: ReactNode;
- jobs: Array<any>;
+ jobs: Array<Job>;
// ...
}
interface BundleAssetsProps extends ComponentProps<typeof Stack> {
- jobs: Array<any>;
+ jobs: Array<Job>;
// ...
}Also applies to: 112-112
| const filterFieldsData = useMemo( | ||
| () => getFilters({ compareMode: jobs?.length > 1, filters, chunks }), |
There was a problem hiding this comment.
Add missing dependency 'chunks' in 'useMemo' hook
The useMemo hook depends on chunks, but chunks is missing from the dependency array. This might lead to stale values when chunks change.
Apply this diff to fix the dependency array:
const filterFieldsData = useMemo(
() => getFilters({ compareMode: jobs?.length > 1, filters, chunks }),
- [jobs, filters],
+ [jobs, filters, chunks],
);Committable suggestion skipped: line range outside the PR's diff.
| const handleExportClick = useCallback((sourceType: string) => { | ||
| setExportSourceType(sourceType); | ||
| exportDialog.toggle(); |
There was a problem hiding this comment.
Include 'exportDialog' in 'handleExportClick' dependencies
The handleExportClick function uses exportDialog.toggle, but exportDialog is not included in the dependencies array of the useCallback hook. This could cause unexpected behavior if exportDialog changes.
Apply this diff to fix the dependency array:
const handleExportClick = useCallback((sourceType: string) => {
setExportSourceType(sourceType);
exportDialog.toggle();
-}, []);
+}, [exportDialog]);
vio
force-pushed
the
assets-chunks-filter
branch
from
e3fc3ed
to
0615a90
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (18)
packages/ui/src/hooks/rows-sort.ts (2)
8-21: Consider enhancing type safety and documentation.The function implementation is correct, but could be improved in the following ways:
/** * Get sort direction field from a string param + * @param directionParam - The direction parameter to validate + * @returns '' for invalid/undefined input, 'asc'|'desc' for valid input */ -const getSortDirection = (directionParam: string | undefined): SortAction['direction'] => { +const getSortDirection = (directionParam: string | undefined): '' | 'asc' | 'desc' => { if (typeof directionParam === 'undefined') { return ''; } - if (['asc', 'desc'].includes(directionParam)) { + if (directionParam === 'asc' || directionParam === 'desc') { - return directionParam as SortAction['direction']; + return directionParam; } return ''; };
Line range hint
74-78: Consider moving the comment for better readability.The explanatory comment about empty direction defaulting to ascending order would be more helpful if placed before the
orderBycall.const orderedRows = useMemo( - () => + () => { + // if direction is empty (reset), sort asc + const effectiveDirection = sort.direction !== '' ? sort.direction : SORT.ASC; + return orderBy( rows, getSortFn(sort.field, getCustomSort), - // if direction is empty (reset), sort asc - sort.direction !== '' ? sort.direction : (SORT.ASC as any), + effectiveDirection as any, ), + } [rows, sort], );packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (2)
25-34: Consider adding component documentationWhile the meta configuration is correct, consider adding a description and parameter documentation to improve the component's documentation in Storybook:
const meta: Meta<typeof BundleAssets> = { title: 'Components/BundleAssets', component: BundleAssets, decorators: [getWrapperDecorator()], args: { setState: action('STATE'), }, + parameters: { + docs: { + description: { + component: 'BundleAssets component displays and manages asset information.', + }, + }, + }, };
Line range hint
38-99: Enhance story organization and documentationWhile the stories cover various scenarios, consider the following improvements:
- Add descriptions to each story:
export const Default: Story = { + parameters: { + docs: { + description: { + story: 'Displays bundle assets with baseline job only.', + }, + }, + }, args: { jobs: [baselineJob], }, };
- Consider extracting test data setup:
// test-data.ts export const createTestJobs = { emptyAssets: () => JOBS.map((job) => set(merge({}, job), 'metrics.webpack.assets', {}) ), notPredictive: () => [ // ... extracted from NotPredictive story ], // ... other test data factories };
- Simplify data manipulation in stories like
EmptyFilteredDataandNotPredictiveby using the extracted test data.packages/ui/src/components/bundle-assets/index.tsx (4)
23-30: Improve type definition for setState propThe
setStatetype is currently too generic. Consider defining a more specific type that reflects the actual state structure and possible mutations.export type BundleAssetsProps = { jobs: Array<Job>; filters?: Record<string, boolean>; search?: string; sortBy?: string; direction?: string; - setState: () => void; + setState: (params: { search?: string; filters?: Record<string, boolean>; sortBy?: string; direction?: string }) => void; };
33-41: Consider explicit default values instead of undefinedUsing
undefinedas default values might lead to unnecessary undefined checks downstream. Consider using more meaningful defaults.const { jobs, - filters = undefined, - search = undefined, - sortBy = undefined, - direction = undefined, + filters = {}, + search = '', + sortBy = '', + direction = 'desc', setState, ...restProps } = props;
43-44: Consider memoizing chunkIds separatelyThe
chunkIdsderivation could be memoized separately if it's used independently ofchunksto prevent unnecessary recalculations.- const { chunks, chunkIds } = useMemo(() => getJobsChunksData(jobs), [jobs]); + const { chunks, chunkIds } = useMemo(() => getJobsChunksData(jobs), [jobs]); + const memoizedChunkIds = useMemo(() => chunkIds, [chunkIds]);
99-107: Consider splitting the component props spreadThe spread of
restPropsbeforesearchPropsmight accidentally override important search-related properties. Consider reorganizing the props order.return ( <BundleAssetsComponent - {...restProps} {...searchProps} + {...restProps} jobs={jobs} chunks={chunks} sort={sortProps.sort} items={sortProps.items} allItems={rows} totalRowCount={totalRowCount} updateSort={sortProps.updateSort} hideEntryInfo={hideEntryInfo} showEntryInfo={showEntryInfo} /> );packages/ui/src/components/sort-button/sort-button.tsx (1)
Line range hint
39-45: LGTM! Consider adding JSDoc comments.The exported interface improves reusability and type safety. The properties are well-structured and their usage is clear from the implementation.
Consider adding JSDoc comments to document the interface and its properties:
+/** + * Props for the SortButton component + */ export interface SortButtonProps { + /** Path to the field in the data structure */ fieldPath: string; + /** Name of the field to sort by */ fieldName: string; + /** Human-readable label for the field */ label: string; + /** Current sort state */ sort?: SortAction; + /** Callback to update sort state */ updateSort?: (params: SortAction) => void; }packages/ui/src/components/metrics-table-header/metrics-table-header.tsx (4)
56-57: Add JSDoc comments for better documentation.The type improvements using
SortButtonPropsare good. Consider adding JSDoc comments to document these props:interface ColumnSumProps { rows: Array<ReportMetricRow>; isBaseline: boolean; runIndex: number; + /** Sort configuration inherited from SortButton */ sort?: SortButtonProps['sort']; + /** Callback to update sort configuration */ updateSort?: SortButtonProps['updateSort']; }
Line range hint
113-134: Fix inconsistent typing for sort-related props.While the interface export and documentation are good improvements, there's an inconsistency in the typing. The sort-related props are still using
anytype whileColumnSumPropshas been updated to useSortButtonProps.Apply this diff to maintain consistent typing:
export interface MetricsTableHeaderProps { /** * Metric column title */ metricTitle?: React.ReactNode; /** * Show column sum flag */ showSum?: boolean; /** * Array of jobs */ jobs: Array<{ label: string; internalBuildNumber: number }>; /** * Report rows */ rows: Array<ReportMetricRow>; /** * Sort field data */ - sort?: any; + sort?: SortButtonProps['sort']; /** * Update sort field */ - updateSort?: (val: any) => void; + updateSort?: SortButtonProps['updateSort']; }
Line range hint
152-154: Add missing key prop in JobColumn mapping.React requires a unique key prop when rendering arrays of elements. The JobColumn mapping is missing this key.
Apply this diff:
{jobs.map((job, runIndex) => ( - <JobColumn job={job} isBaseline={runIndex === jobs.length - 1} /> + <JobColumn + key={`${job.label}-${job.internalBuildNumber}`} + job={job} + isBaseline={runIndex === jobs.length - 1} + /> ))}
Line range hint
158-166: Add missing key prop in SumColumn mapping.Similar to the JobColumn issue, the SumColumn mapping is missing a key prop.
Apply this diff:
{jobs.map((_, runIndex) => ( <SumColumn + key={runIndex} rows={rows} isBaseline={runIndex === jobs.length - 1} runIndex={runIndex} updateSort={updateSort} sort={sort} /> ))}packages/ui/src/components/metrics-table/metrics-table.tsx (1)
77-78: LGTM! Consider adding JSDoc commentsThe type improvements for
sortandupdateSortproperties enhance type safety by ensuring alignment with the header component's props. This is a good practice that prevents potential type mismatches.Consider adding JSDoc comments to document these properties:
+ /** Sort configuration matching the header component's sort prop */ sort?: MetricsTableHeaderProps['sort']; + /** Callback to update sort matching the header component's updateSort prop */ updateSort?: MetricsTableHeaderProps['updateSort'];packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (2)
25-51: Simplify the predictive check logicThe current implementation is harder to follow due to multiple early returns and accumulator mutation.
Consider this more functional approach:
-export const getIsNotPredictive = (row: ReportMetricRow): boolean => { - const { key, runs } = row; - - return runs.reduce((agg, run, index) => { - if (agg) { - return agg; - } - - if (index + 1 === runs.length) { - return agg; - } - - const preRun = runs[index + 1]; - - if ( - run && - preRun && - key !== run.name && - run.name === preRun.name && - run.value !== preRun.value - ) { - return true; - } - - return agg; - }, false); -}; +export const getIsNotPredictive = (row: ReportMetricRow): boolean => { + const { key, runs } = row; + + return runs.some((run, index) => { + if (index + 1 === runs.length) return false; + + const nextRun = runs[index + 1]; + return ( + run && + nextRun && + key !== run.name && + run.name === nextRun.name && + run.value !== nextRun.value + ); + }); +};
133-140: Improve sort logic readability with named constantsThe boolean expressions could be more self-documenting.
Consider this enhancement:
+const SORT_PRIORITIES = { + PREDICTIVE: 'predictive', + UNCHANGED: 'unchanged', + NON_INITIAL: 'non_initial', + NON_ENTRY: 'non_entry', + NON_CHUNK: 'non_chunk', + KEY: 'key', +} as const; + export const getCustomSort = (item: ReportMetricAssetRow): Array<boolean | string> => [ - !item.isNotPredictive, - !item.changed, - !item.isInitial, - !item.isEntry, - !item.isChunk, - item.key, + !item.isNotPredictive, // SORT_PRIORITIES.PREDICTIVE + !item.changed, // SORT_PRIORITIES.UNCHANGED + !item.isInitial, // SORT_PRIORITIES.NON_INITIAL + !item.isEntry, // SORT_PRIORITIES.NON_ENTRY + !item.isChunk, // SORT_PRIORITIES.NON_CHUNK + item.key, // SORT_PRIORITIES.KEY ];packages/ui/src/components/bundle-assets/bundle-assets.tsx (2)
197-202: Avoid type assertion in renderRowHeaderThe type assertion
as ReportMetricAssetRowcould be avoided by properly typing the generic parameter of ReportMetricRow.- renderRowHeader = useCallback( - (row: ReportMetricRow) => ( + renderRowHeader = useCallback( + (row: ReportMetricRow & ReportMetricAssetRow) => ( <AssetName - row={row as ReportMetricAssetRow} + row={row} EntryComponentLink={EntryComponentLink} className={css.assetName} /> ), [EntryComponentLink], );
Line range hint
270-283: Add error boundaries and loading statesThe MetricsTable and ViewMetricsTreemap components handle complex data transformations but lack error boundaries and loading states. Consider adding error handling for cases where data transformation fails or when the component is in a loading state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
packages/ui/src/components/asset-info/asset-info.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.tsx(10 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.ts(1 hunks)packages/ui/src/components/bundle-assets/index.tsx(4 hunks)packages/ui/src/components/metrics-table-header/metrics-table-header.tsx(3 hunks)packages/ui/src/components/metrics-table/metrics-table.tsx(2 hunks)packages/ui/src/components/sort-button/sort-button.tsx(1 hunks)packages/ui/src/hooks/rows-sort.ts(3 hunks)packages/ui/src/hooks/search-params.js(0 hunks)packages/ui/src/types.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/ui/src/components/bundle-assets/bundle-assets.utils.js
- packages/ui/src/hooks/search-params.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/asset-info/asset-info.tsx
- packages/ui/src/types.d.ts
🔇 Additional comments (15)
packages/ui/src/hooks/rows-sort.ts (2)
36-36: LGTM! Type change aligns with validation approach.
The change from SortAction['direction'] to string makes sense as it allows for flexible input handling, with runtime validation performed by getSortDirection.
57-60: LGTM! State initialization looks good.
The integration of getSortDirection ensures valid sort direction values in the state.
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (3)
11-12: LGTM: Proper TypeScript imports for Storybook
The addition of proper type imports from '@storybook/react' and action from '@storybook/addon-actions' follows best practices for TypeScript Storybook stories.
36-36: LGTM: Proper Story type definition
The Story type is correctly defined using StoryObj with proper generic type parameter.
Line range hint 66-99: Verify deep cloning behavior
The complex data manipulation using merge and set could lead to unintended mutations. Ensure that the test data is properly isolated between stories.
✅ Verification successful
Let me analyze the results and generate the final response.
Deep cloning pattern is consistent and safe
The usage of merge and set follows a consistent pattern across the codebase where:
- Each operation creates a new object using
merge({}, source) - The result is immediately passed to
set()for immutable updates - No intermediate mutations or shared references are present
This pattern effectively isolates test data between stories and prevents unintended mutations because:
merge({}, source)creates a fresh copyset()creates another new object when setting the property
The same pattern is used in:
bundle-assets.stories.tsxbundle-packages.stories.jsxapp.stories.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of merge/set to verify consistent patterns
rg --type typescript "merge\({|}," -A 2
rg --type typescript "set\(merge" -A 2
# Look for potential test isolation issues
ast-grep --pattern 'merge($_, $_)'
Length of output: 4605
packages/ui/src/components/bundle-assets/index.tsx (2)
16-20: Verify the necessity of individual utility function imports
The separate imports of utility functions might indicate a need to review the modularization strategy. Consider whether these functions are used elsewhere and if they should remain as separate exports.
✅ Verification successful
Separate utility functions are correctly modularized
The utility functions are appropriately modularized and used across multiple components:
getCustomSortis used in bundle-assets, bundle-modules, and bundle-packages componentsgenerateGetRowFilteris shared between bundle-assets and bundle-modulesaddMetricReportAssetRowDatais specific to bundle-assets and has dedicated tests
The separate imports are justified as these utilities are part of a well-structured, reusable pattern across bundle-related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these utility functions are used in other files
rg "addMetricReportAssetRowData|generateGetRowFilter|getCustomSort" --type ts --type tsx
Length of output: 118
Script:
#!/bin/bash
# Check if these utility functions are used in other files, using ripgrep without file type filters
rg "addMetricReportAssetRowData|generateGetRowFilter|getCustomSort"
# Also check the implementation location of these functions
ast-grep --pattern 'function $_($$) { $$$ }' --lang typescript packages/ui/src/components/bundle-assets/bundle-assets.utils.ts
Length of output: 3342
78-84: Verify the impact of the new filter generation approach
The switch to generateGetRowFilter represents a significant change in filtering logic. Ensure this change maintains the expected filtering behavior.
✅ Verification successful
No impact concerns with the filter generation approach
The implementation of generateGetRowFilter in bundle-assets.utils.ts follows the same filtering pattern that's already established in the codebase, specifically matching the pattern used in bundle-modules.utils.ts. The function:
- Correctly handles chunk filtering based on checked chunk IDs
- Maintains consistent filtering for changed assets
- Properly filters by asset types (entry, initial, chunk, other)
- Includes file type filtering
- Uses the same intersection logic for chunk filtering as the modules implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for tests covering the filtering logic
rg -t test "generateGetRowFilter|getRowFilter"
# Check for any existing filter-related issues
gh issue list --label "filter" --state all
Length of output: 120
Script:
#!/bin/bash
# Look for tests covering the filtering logic with proper file extensions
rg "generateGetRowFilter|getRowFilter" -g "*.test.*" -g "*.spec.*"
# Look for the implementation of generateGetRowFilter
ast-grep --pattern 'const generateGetRowFilter = $_'
# Look for any usage of generateGetRowFilter in the codebase
rg "generateGetRowFilter" -A 3
Length of output: 12144
packages/ui/src/components/sort-button/sort-button.tsx (2)
Line range hint 46-102: Implementation looks robust and well-structured.
The component implementation:
- Properly handles optional props
- Uses memoization effectively
- Implements clear sorting logic
- Has good accessibility with tooltips
Line range hint 39-45: Verify the interface usage across the codebase.
Let's ensure this interface is properly used where imported.
✅ Verification successful
Let me verify the actual usage of SortButton component to ensure the interface is properly integrated.
Interface is properly integrated and used consistently
The verification shows that:
SortButtonPropsis correctly imported and used inMetricsTableHeader- All
SortButtonusages inMetricsTableHeaderprovide the required props (fieldPath,fieldName,label,sort,updateSort) - The interface is referenced correctly for type inheritance in both the component implementation and the
MetricsTableHeaderprops
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where SortButtonProps is imported and used
# Find direct imports of SortButtonProps
echo "=== Direct imports of SortButtonProps ==="
rg "import.*SortButtonProps.*from.*sort-button'" -A 2
# Find usage of SortButton component to verify prop types
echo -e "\n=== Usage of SortButton component ==="
rg "<SortButton.*>" -A 2
# Find TypeScript type references
echo -e "\n=== Type references to SortButtonProps ==="
rg ": SortButtonProps" -A 2
Length of output: 1510
Script:
#!/bin/bash
# Check the implementation in metrics-table-header.tsx
echo "=== MetricsTableHeader Implementation ==="
rg -A 20 "<SortButton" packages/ui/src/components/metrics-table-header/metrics-table-header.tsx
# Check for any other potential usages of SortButton
echo -e "\n=== All SortButton usages with context ==="
ast-grep --pattern '<SortButton $$$>'
Length of output: 4383
packages/ui/src/components/metrics-table-header/metrics-table-header.tsx (1)
15-15: LGTM! Good type safety improvement.
Explicitly importing and using SortButtonProps type improves type safety and provides better IDE support.
packages/ui/src/components/metrics-table/metrics-table.tsx (1)
12-12: LGTM! Import enhancement improves type safety
The explicit import of MetricsTableHeaderProps is a good practice, making the type dependencies clear and enabling stronger type checking for the sort-related properties.
packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (4)
1-21: LGTM: Well-structured imports with proper type definitions
The imports are well-organized, using specific lodash utilities instead of the entire library, and properly importing TypeScript types.
56-84: LGTM: Clear and well-typed status determination
The function has clear logic with appropriate early returns and good type safety.
89-127: Previous review comment is still applicable
The concerns about type safety and code complexity raised in the previous review remain valid.
142-215: LGTM: Well-structured filter configuration
The filter configuration is well-implemented with proper type safety and default value handling.
| export const generateGetRowFilter = | ||
| ({ chunkIds }: GenerateGetRowFilterOptions) => (filters: Record<string, unknown>) => { | ||
| // List of chunkIds with filter value set to `true` | ||
| const checkedChunkIds: Array<string> = []; | ||
|
|
||
| chunkIds.forEach((chunkId) => { | ||
| if (filters[`${ASSET_CHUNK}.${chunkId}`]) { | ||
| checkedChunkIds.push(chunkId); | ||
| } | ||
| }); | ||
|
|
||
| const hasChunkFilters = | ||
| checkedChunkIds.length > 0 && checkedChunkIds.length !== chunkIds.length; | ||
|
|
||
| return (item: ReportMetricAssetRow) => { | ||
| if (filters[ASSET_FILTERS.CHANGED] && !item.changed) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( | ||
| !( | ||
| (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) || | ||
| (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) || | ||
| (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) || | ||
| (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset) | ||
| ) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!filters[`${ASSET_FILE_TYPE}.${item.fileType}`]) { | ||
| return false; | ||
| } | ||
|
|
||
| // Filter if any of the chunkIds are checked | ||
| if (hasChunkFilters) { | ||
| const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || []; | ||
| return intersection(rowRunsChunkIds, checkedChunkIds).length > 0; | ||
| } | ||
|
|
||
| return true; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the filter logic and improving performance
The current implementation is complex and could benefit from better organization and performance optimization.
Consider these improvements:
- Break down the filter logic into smaller, focused functions
- Cache filter results for better performance
export const generateGetRowFilter =
({ chunkIds }: GenerateGetRowFilterOptions) => (filters: Record<string, unknown>) => {
+ // Cache chunk filter results
+ const checkedChunkIds = useMemo(() => {
+ const ids: Array<string> = [];
+ chunkIds.forEach((chunkId) => {
+ if (filters[`${ASSET_CHUNK}.${chunkId}`]) {
+ ids.push(chunkId);
+ }
+ });
+ return ids;
+ }, [chunkIds, filters]);
+
+ const hasChunkFilters = checkedChunkIds.length > 0 && checkedChunkIds.length !== chunkIds.length;
+
+ // Break down filter checks into smaller functions
+ const matchesChangeFilter = (item: ReportMetricAssetRow) =>
+ !filters[ASSET_FILTERS.CHANGED] || item.changed;
+
+ const matchesEntryTypeFilter = (item: ReportMetricAssetRow) =>
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) ||
+ (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset);
+
+ const matchesFileTypeFilter = (item: ReportMetricAssetRow) =>
+ Boolean(filters[`${ASSET_FILE_TYPE}.${item.fileType}`]);
+
+ const matchesChunkFilter = (item: ReportMetricAssetRow) => {
+ if (!hasChunkFilters) return true;
+ const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || [];
+ return intersection(rowRunsChunkIds, checkedChunkIds).length > 0;
+ };
+
return (item: ReportMetricAssetRow) => {
- if (filters[ASSET_FILTERS.CHANGED] && !item.changed) {
- return false;
- }
-
- if (
- !(
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.ENTRY}`] && item.isEntry) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.INITIAL}`] && item.isInitial) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.CHUNK}`] && item.isChunk) ||
- (filters[`${ASSET_ENTRY_TYPE}.${ASSET_FILTERS.OTHER}`] && item.isAsset)
- )
- ) {
- return false;
- }
-
- if (!filters[`${ASSET_FILE_TYPE}.${item.fileType}`]) {
- return false;
- }
-
- if (hasChunkFilters) {
- const rowRunsChunkIds = item?.runs?.map((run) => run?.chunkId) || [];
- return intersection(rowRunsChunkIds, checkedChunkIds).length > 0;
- }
-
- return true;
+ return (
+ matchesChangeFilter(item) &&
+ matchesEntryTypeFilter(item) &&
+ matchesFileTypeFilter(item) &&
+ matchesChunkFilter(item)
+ );
};
};Committable suggestion skipped: line range outside the PR's diff.
| const filterFieldsData = useMemo( | ||
| () => getFilters({ compareMode: jobs?.length > 1, filters, chunks }), | ||
| [jobs, filters], | ||
| ); |
There was a problem hiding this comment.
Implement getFilters function
The getFilters function is imported from the removed bundle-assets.utils.js file. This function needs to be reimplemented here or moved to a new utility file.
Would you like me to help implement the getFilters function based on the existing parameters?
| items: Array<ReportMetricAssetRow>; | ||
| allItems: Array<ReportMetricAssetRow>; | ||
| filters: Record<string, boolean>; | ||
| updateFilters: (newFilters: any) => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for updateFilters prop
The updateFilters callback is typed with any which reduces type safety. It should match the type of the filters prop.
- updateFilters: (newFilters: any) => void;
+ updateFilters: (newFilters: Record<string, boolean>) => void;📝 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.
| updateFilters: (newFilters: any) => void; | |
| updateFilters: (newFilters: Record<string, boolean>) => void; |
vio
force-pushed
the
assets-chunks-filter
branch
from
0615a90
to
44b312e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
packages/ui/src/hooks/rows-sort.ts (2)
8-21: Enhance documentation and type safety of getSortDirection.The implementation is correct and handles edge cases well. Consider these improvements:
+/** Valid sort direction values */ +const VALID_SORT_DIRECTIONS = ['asc', 'desc'] as const; +type ValidSortDirection = typeof VALID_SORT_DIRECTIONS[number]; /** - * Get sort direction field from a string param + * Validates and converts a string parameter into a sort direction. + * @param directionParam - The direction parameter to validate ('asc' or 'desc') + * @returns A valid sort direction or empty string if invalid */ const getSortDirection = (directionParam: string | undefined): SortAction['direction'] => { if (typeof directionParam === 'undefined') { return ''; } - if (['asc', 'desc'].includes(directionParam)) { + if (VALID_SORT_DIRECTIONS.includes(directionParam as ValidSortDirection)) { return directionParam as SortAction['direction']; } return ''; };
Line range hint
74-78: Improve comment clarity for direction fallback.The implementation is correct, but the comment could be more precise.
- // if direction is empty (reset), sort asc + // Default to ascending order when direction is empty (typically after reset) sort.direction !== '' ? sort.direction : (SORT.ASC as any),packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (1)
25-31: Consider adding component description in metaThe meta configuration is well-structured and follows best practices. Consider adding a
parameters.docs.descriptionto provide component documentation directly in Storybook.const meta: Meta<typeof BundleAssets> = { title: 'Components/BundleAssets', component: BundleAssets, decorators: [getWrapperDecorator()], args: { setState: action('STATE'), }, + parameters: { + docs: { + description: { + component: 'BundleAssets component displays...' + } + } + } };packages/ui/src/components/bundle-assets/index.tsx (2)
23-30: Consider improving the setState type definitionThe
setStatetype could be more specific than() => void. Consider defining the exact shape of the state being updated to improve type safety and documentation.export type BundleAssetsProps = { jobs: Array<Job>; filters?: Record<string, boolean>; search?: string; sortBy?: string; direction?: string; - setState: () => void; + setState: (state: { search?: string; filters?: Record<string, boolean>; sortBy?: string; direction?: string }) => void; };
78-83: Consider memoizing the filter generatorThe
generateGetRowFilterfunction is called on every render. Consider memoizing it since it only depends onchunkIds.+ const getRowFilter = useMemo( + () => generateGetRowFilter({ chunkIds }), + [chunkIds] + ); const filteredRows = useRowsFilter({ rows, searchPattern: searchProps.searchPattern, filters: searchProps.filters, - getRowFilter: generateGetRowFilter({ chunkIds }), + getRowFilter, });packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (3)
22-51: Consider simplifying the predictive check logicThe current implementation is complex and could be made more readable by:
- Using more descriptive variable names
- Simplifying the reduce logic
- Extracting the comparison logic into a separate function
Consider this refactoring:
export const getIsNotPredictive = (row: ReportMetricRow): boolean => { const { key, runs } = row; - return runs.reduce((agg, run, index) => { - if (agg) { - return agg; - } - - if (index + 1 === runs.length) { - return agg; - } - - const preRun = runs[index + 1]; - - if ( - run && - preRun && - key !== run.name && - run.name === preRun.name && - run.value !== preRun.value - ) { - return true; - } - - return agg; - }, false); + const isRunUnpredictable = (currentRun: AssetMetricRun | null, nextRun: AssetMetricRun | null) => + currentRun && + nextRun && + key !== currentRun.name && + currentRun.name === nextRun.name && + currentRun.value !== nextRun.value; + + // Check consecutive runs for unpredictable changes + for (let i = 0; i < runs.length - 1; i++) { + if (isRunUnpredictable(runs[i], runs[i + 1])) { + return true; + } + } + + return false; };
52-84: Consider a more descriptive function nameThe current name
getAssetMetaStatuscould be more specific about what it does. Consider renaming to something likegetAssetChangeStatusordetermineAssetStateChangeto better reflect its purpose of tracking additions, removals, and changes.
133-140: Improve clarity of sorting priority logicThe current implementation uses boolean negations which makes the priority order less obvious. Consider using an enum or constants to make the sorting priority more explicit.
+const enum SortPriority { + PREDICTIVE = 1, + UNCHANGED = 2, + NON_INITIAL = 3, + NON_ENTRY = 4, + NON_CHUNK = 5 +} + export const getCustomSort = (item: ReportMetricAssetRow): Array<number | string> => [ - !item.isNotPredictive, - !item.changed, - !item.isInitial, - !item.isEntry, - !item.isChunk, + item.isNotPredictive ? 0 : SortPriority.PREDICTIVE, + item.changed ? 0 : SortPriority.UNCHANGED, + item.isInitial ? 0 : SortPriority.NON_INITIAL, + item.isEntry ? 0 : SortPriority.NON_ENTRY, + item.isChunk ? 0 : SortPriority.NON_CHUNK, item.key, ];packages/ui/src/components/bundle-assets/bundle-assets.tsx (3)
228-228: Add proper typing for exportSourceType stateThe
exportSourceTypestate lacks proper typing. Consider defining an enum or union type for the possible source types.Example:
type ExportSourceType = 'csv' | 'json' | 'markdown'; // adjust based on actual source types const [exportSourceType, setExportSourceType] = useState<ExportSourceType>();
Line range hint
94-100: Make boolean conversion explicitThe boolean coercion of
displayType.groupByis implicit. Consider making it more explicit for better readability.<MetricsTreemap treeNodes={treeNodes} - nested={Boolean(displayType.groupBy)} + nested={displayType.groupBy !== undefined} emptyMessage={emptyMessage} onItemClick={showEntryInfo} onGroupClick={onGroupClick}
75-76: Fix typo in commentThere's a typo in the comment: "beggining" should be "beginning".
- // Clear seach when groupPath is emty (root) - // Search by group path - // 1. use `^` to match only the string beggining + // Clear search when groupPath is empty (root) + // Search by group path + // 1. use `^` to match only the string beginning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/ui/src/components/asset-info/asset-info.tsx(3 hunks)packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts(2 hunks)packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.tsx(10 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.ts(1 hunks)packages/ui/src/components/bundle-assets/index.tsx(4 hunks)packages/ui/src/components/metrics-table-header/metrics-table-header.tsx(3 hunks)packages/ui/src/components/metrics-table/metrics-table.tsx(2 hunks)packages/ui/src/components/sort-button/sort-button.tsx(1 hunks)packages/ui/src/hooks/rows-sort.ts(3 hunks)packages/ui/src/hooks/search-params.js(0 hunks)packages/ui/src/types.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/ui/src/components/bundle-assets/bundle-assets.utils.js
- packages/ui/src/hooks/search-params.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/ui/src/components/asset-info/asset-info.tsx
- packages/ui/src/components/metrics-table-header/metrics-table-header.tsx
- packages/ui/src/components/metrics-table/metrics-table.tsx
- packages/ui/src/components/sort-button/sort-button.tsx
- packages/ui/src/types.d.ts
🧰 Additional context used
🪛 Biome
packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts
[error] 86-86: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (13)
packages/ui/src/hooks/rows-sort.ts (2)
36-36: LGTM! Type change improves API flexibility.
The change to accept any string for initialDirection is a good decision as it:
- Provides a more flexible public API
- Maintains type safety through runtime validation in
getSortDirection - Follows React conventions where props are typically strings
57-60: LGTM! State initialization is correct.
The sort state initialization properly integrates with the new validation logic.
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (3)
11-12: LGTM: Appropriate TypeScript imports for Storybook
The addition of type imports from '@storybook/react' and the action creator from '@storybook/addon-actions' follows best practices for TypeScript-based Storybook files.
36-36: LGTM: Proper story type definition
The Story type is correctly defined using StoryObj with proper component type reference.
Line range hint 38-99: Add documentation for each story scenario
While the stories cover various scenarios, they would benefit from explicit documentation describing what each story demonstrates and its expected behavior.
export const Default: Story = {
+ parameters: {
+ docs: {
+ description: 'Displays bundle assets with baseline data only'
+ }
+ },
args: {
jobs: [baselineJob],
},
};Verify test data integrity
The stories use various test data manipulations. Let's verify the consistency of the test fixtures.
packages/ui/src/components/bundle-assets/index.tsx (3)
32-41: LGTM! Props handling follows best practices
The props destructuring with explicit undefined defaults is well implemented, allowing proper distinction between unset and explicitly set values.
99-107: LGTM! Props spreading follows best practices
The props spreading order ensures that specific props override any conflicting props from restProps, which is the correct pattern.
43-44: Verify the impact of chunks data structure change
The destructuring of chunks and chunkIds from getJobsChunksData suggests a change in the returned data structure.
✅ Verification successful
No changes needed in data structure usage
The verification shows that getJobsChunksData is consistently used across both bundle-assets and bundle-modules components, with identical destructuring pattern of { chunks, chunkIds }. The function implementation in jobs.ts confirms this is the expected return structure, with no recent changes that would impact existing usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of getJobsChunksData to ensure compatibility
rg "getJobsChunksData" --type ts --type tsx
Length of output: 71
Script:
#!/bin/bash
# Search for getJobsChunksData function definition and usages
rg "getJobsChunksData" -t typescript -t javascriptreact -t typescriptreact
# Also search for the function definition using AST to understand the data structure
ast-grep --pattern 'function getJobsChunksData($_) {
$$$
}'
Length of output: 173
Script:
#!/bin/bash
# Search for getJobsChunksData function definition and usages with just file extension
rg "getJobsChunksData" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"
# Find all files that might contain this function
fd -e ts -e tsx -e js -e jsx | xargs grep -l "getJobsChunksData"
Length of output: 837
Script:
#!/bin/bash
# Let's examine the function definition and its return type
rg "getJobsChunksData" -g "*.ts" -A 10 packages/ui/src/utils/jobs.ts
# Also check the usage pattern in bundle-modules to compare with bundle-assets
rg "getJobsChunksData" -g "*.tsx" -B 2 -A 2 packages/ui/src/components/bundle-modules/index.tsx
Length of output: 970
packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts (1)
Line range hint 1-1: Verify the import statement's validity.
The import statement references bundle-assets.utils.js, but according to the PR summary, this file has been removed. This could lead to runtime errors.
Let's verify if the utility file still exists and if the function has been moved elsewhere:
✅ Verification successful
Let me verify the function's existence in the utility file.
Import statement is valid and function exists
The bundle-assets.utils.ts file exists at the correct location and contains the addMetricReportAssetRowData function. The import statement is valid and will not cause runtime errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the utility file exists
fd -e js -e ts "bundle-assets.utils"
# Search for the function definition in case it was moved
ast-grep --pattern 'function addMetricReportAssetRowData'
Length of output: 160
Script:
#!/bin/bash
# Check the content of the utils file for the function
rg "addMetricReportAssetRowData" packages/ui/src/components/bundle-assets/bundle-assets.utils.ts -A 2
Length of output: 228
packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (3)
1-21: LGTM! Well-organized imports following best practices
The imports are properly organized with:
- Specific lodash utility imports for better tree-shaking
- Clear separation between type and value imports
- Proper use of TypeScript type imports
85-127: Previous review comment is still applicable
The existing review comment about improving type safety and reducing complexity is still valid for this function.
142-260: Previous review comment is still applicable
The existing review comment about breaking down the filter logic and improving performance through memoization is still valid.
packages/ui/src/components/bundle-assets/bundle-assets.tsx (1)
32-32:
Fix import from removed file
The getFilters function is imported from bundle-assets.utils.js, but according to the AI summary, this file has been removed.
This was already flagged in a previous review. Let me know if you need help implementing the getFilters function.
| @@ -83,7 +83,7 @@ describe('BundleAssets / addMetricReportAssetRowData', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| test('should add add data when curent run is null', () => { | |||
| test.only('should add data when current run is null', () => { | |||
There was a problem hiding this comment.
Remove .only modifier from the test case.
The .only modifier will cause Jest to run only this specific test case while skipping all others. This is typically used during development for debugging purposes and should not be committed to the codebase.
Apply this diff to fix the issue:
- test.only('should add data when current run is null', () => {
+ test('should add data when current run is null', () => {📝 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.
| test.only('should add data when current run is null', () => { | |
| test('should add data when current run is null', () => { |
🧰 Tools
🪛 Biome
[error] 86-86: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
vio
force-pushed
the
assets-chunks-filter
branch
from
44b312e
to
00fdf0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/ui/src/hooks/rows-sort.ts (1)
8-21: Consider enhancing type safety and documentationThe implementation is correct and handles all cases well. However, consider these improvements:
- Define valid directions as a const enum or object to avoid magic strings
- Enhance JSDoc to document the return values and their meaning
+const VALID_DIRECTIONS = { + ASC: 'asc', + DESC: 'desc', +} as const; /** * Get sort direction field from a string param + * @param directionParam - Optional direction parameter + * @returns '' for undefined/invalid input, 'asc'|'desc' for valid input */ const getSortDirection = (directionParam: string | undefined): SortAction['direction'] => { if (typeof directionParam === 'undefined') { return ''; } - if (['asc', 'desc'].includes(directionParam)) { + if (Object.values(VALID_DIRECTIONS).includes(directionParam)) { return directionParam as SortAction['direction']; } return ''; };packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (2)
25-34: LGTM: Well-structured meta configuration with global argsMoving
setStateto meta args is a good practice that reduces duplication across stories.Consider extracting the args type to make it reusable:
type BundleAssetsArgs = { setState: (params: any) => void; jobs?: Array<any>; filters?: Record<string, boolean>; search?: string; }; const meta: Meta<typeof BundleAssets> = { // ... args: { setState: action('STATE'), } satisfies Partial<BundleAssetsArgs>, };
Line range hint
38-99: Consider improving type safety for test dataThe stories are well-structured and cover various scenarios. However, the test data manipulation could benefit from stronger typing.
Consider adding types for the test data and utilities:
interface WebpackStats { assets: Record<string, { name?: string; value: number; }>; } interface Job { metrics: { webpack: WebpackStats; }; } // Then use these types: export const NoAssets: Story = { args: { jobs: JOBS.map((job: Job) => set(merge({}, job), 'metrics.webpack.assets', {}) ), }, };This would make the test data manipulation more type-safe and catch potential issues during development.
packages/ui/src/components/bundle-assets/index.tsx (2)
23-30: Improve type definition for setState prop.The current type definition
() => voidforsetStateis too generic. Consider defining a more specific type that reflects its actual usage for updating search parameters and sort state.export type BundleAssetsProps = { jobs: Array<Job>; filters?: Record<string, boolean>; search?: string; sortBy?: string; direction?: string; - setState: () => void; + setState: (params: { search?: string; filters?: Record<string, boolean>; sortBy?: string; direction?: string }) => void; };
98-108: Consider grouping related props.The props spread and individual prop assignments could be organized better for readability.
return ( <BundleAssetsComponent - {...restProps} - {...searchProps} - jobs={jobs} - chunks={chunks} - sort={sortProps.sort} - items={sortProps.items} - allItems={rows} - totalRowCount={totalRowCount} - updateSort={sortProps.updateSort} - hideEntryInfo={hideEntryInfo} - showEntryInfo={showEntryInfo} + // Base props + {...restProps} + jobs={jobs} + chunks={chunks} + // Search and filter props + {...searchProps} + // Sort props + sort={sortProps.sort} + updateSort={sortProps.updateSort} + // Data props + items={sortProps.items} + allItems={rows} + totalRowCount={totalRowCount} + // Entry info props + hideEntryInfo={hideEntryInfo} + showEntryInfo={showEntryInfo} /> );packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (1)
25-51: Consider simplifying the predictive check logicThe current implementation using reduce could be simplified for better readability and type safety.
Consider this alternative implementation:
export const getIsNotPredictive = (row: ReportMetricRow): boolean => { const { key, runs } = row; - - return runs.reduce((agg, run, index) => { - if (agg) { - return agg; - } - - if (index + 1 === runs.length) { - return agg; - } - - const preRun = runs[index + 1]; - - if ( - run && - preRun && - key !== run.name && - run.name === preRun.name && - run.value !== preRun.value - ) { - return true; - } - - return agg; - }, false); + // Compare adjacent runs + for (let i = 0; i < runs.length - 1; i++) { + const currentRun = runs[i]; + const nextRun = runs[i + 1]; + + if (!currentRun || !nextRun) continue; + + if ( + key !== currentRun.name && + currentRun.name === nextRun.name && + currentRun.value !== nextRun.value + ) { + return true; + } + } + + return false; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/ui/src/components/asset-info/asset-info.tsx(3 hunks)packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts(2 hunks)packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx(3 hunks)packages/ui/src/components/bundle-assets/bundle-assets.tsx(10 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.ts(1 hunks)packages/ui/src/components/bundle-assets/index.tsx(4 hunks)packages/ui/src/components/metrics-table-header/metrics-table-header.tsx(3 hunks)packages/ui/src/components/metrics-table/metrics-table.tsx(2 hunks)packages/ui/src/components/sort-button/sort-button.tsx(1 hunks)packages/ui/src/hooks/rows-sort.ts(3 hunks)packages/ui/src/hooks/search-params.js(0 hunks)packages/ui/src/types.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/ui/src/components/bundle-assets/bundle-assets.utils.js
- packages/ui/src/hooks/search-params.js
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/ui/src/components/asset-info/asset-info.tsx
- packages/ui/src/components/bundle-assets/tests/add-metric-report-asset-row-data.ts
- packages/ui/src/components/metrics-table-header/metrics-table-header.tsx
- packages/ui/src/components/metrics-table/metrics-table.tsx
- packages/ui/src/components/sort-button/sort-button.tsx
- packages/ui/src/types.d.ts
🔇 Additional comments (18)
packages/ui/src/hooks/rows-sort.ts (2)
36-36: Type change is safe due to runtime validation
The type change from SortAction['direction'] to string is acceptable since the getSortDirection function properly validates the input at runtime.
57-60: LGTM: Proper integration of sort direction validation
The sort state initialization correctly uses getSortDirection to validate the initial direction, ensuring consistent and valid sort state throughout the component lifecycle.
✅ Verification successful
LGTM: Sort direction handling is consistent and well-integrated
The verification confirms that:
- Sort direction validation is properly implemented through
getSortDirection - The
SORTconstant is used consistently across the codebase for 'asc'/'desc' values - Type safety is enforced via the
SortActioninterface - Default sorting behavior falls back to 'asc' when direction is empty
- All sort-related components use the same type definitions and constants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of sort direction across the codebase
# to ensure consistent handling
# Check for any direct usage of 'asc'/'desc' strings that might bypass validation
rg -t typescript -t javascript "direction: ['\"](?:asc|desc)['\"]" -A 2
# Look for other sort-related utilities that might need similar validation
ast-grep --pattern 'function $FUNC($PARAMS) {
$$$
sort($$$)
$$$
}'
Length of output: 187
Script:
#!/bin/bash
# Let's try a different approach to verify sort direction usage
# Search for sort direction related code
rg --type-add 'ts:*.{ts,tsx}' --type ts "direction.*(?:asc|desc)" -A 2
# Look for getSortDirection usage
rg --type-add 'ts:*.{ts,tsx}' --type ts "getSortDirection" -A 2
# Find sort-related state or type definitions
rg --type-add 'ts:*.{ts,tsx}' --type ts "(?:type|interface).*Sort.*{" -A 5
Length of output: 2078
Script:
#!/bin/bash
# Let's check the sort-related type definitions with a corrected pattern
# Find sort-related type definitions
rg --type-add 'ts:*.{ts,tsx}' --type ts "(type|interface).*Sort.*\{" -A 5
# Check for SORT constant definition and usage
rg --type-add 'ts:*.{ts,tsx}' --type ts "SORT\s*=\s*" -A 2
# Look at the complete rows-sort.ts file for context
cat packages/ui/src/hooks/rows-sort.ts
Length of output: 4443
packages/ui/src/components/bundle-assets/bundle-assets.stories.tsx (2)
11-12: LGTM: Proper TypeScript and Storybook imports
The addition of TypeScript types from Storybook and the action addon follows best practices for typed stories.
36-36: LGTM: Proper Story type definition
The Story type is correctly defined using StoryObj with the component type reference.
packages/ui/src/components/bundle-assets/index.tsx (4)
3-3: LGTM! Well-organized imports with proper type definitions.
The modularization of utilities and addition of TypeScript types improves code organization and type safety.
Also applies to: 16-21
Line range hint 87-94: LGTM! Improved type safety in sorting implementation.
The addition of the ReportMetricAssetRow generic type to useRowsSort enhances type safety and makes the code more maintainable.
78-84: Verify the new filtering implementation.
The filtering logic has been updated to use generateGetRowFilter. Ensure that all existing filter scenarios are still handled correctly.
#!/bin/bash
# Look for test coverage of the new filter implementation
rg --type ts --type tsx "generateGetRowFilter.+(test|spec)" -A 10
# Check if there are any existing filter scenarios in tests
rg --type ts --type tsx "describe.*filter" -A 10 packages/ui/src/components/bundle-assets/__tests__/44-44: Verify the memoization dependency array.
The useMemo dependency only includes jobs, but getJobsChunksData is imported. Ensure that getJobsChunksData is either pure or included in the dependencies if it can change.
✅ Verification successful
The useMemo dependency array is correctly implemented
The getJobsChunksData function is a pure function that only depends on its input parameter. It's defined in utils/jobs.ts and only uses lodash/uniqBy for data transformation. The current dependency array [jobs] is sufficient as the function itself is stable and won't change between renders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getJobsChunksData is defined in the same file or imported
rg --type ts --type tsx "export.+getJobsChunksData" -A 5
Length of output: 226
Script:
#!/bin/bash
# Search for getJobsChunksData function definition or import
rg "getJobsChunksData" --type typescript --type tsx
# Also try to find the function definition using ast-grep
ast-grep --pattern 'function getJobsChunksData($_) {
$$$
}'
# And check for any exports of this function
ast-grep --pattern 'export function getJobsChunksData($_) {
$$$
}'
# Check for const exports
ast-grep --pattern 'export const getJobsChunksData = $_'
Length of output: 280
Script:
#!/bin/bash
# Try with just ripgrep without type specification
rg "getJobsChunksData"
# Also search for the file content to locate the function
rg "const.*getJobsChunksData"
Length of output: 890
Script:
#!/bin/bash
# Let's check the implementation of getJobsChunksData in utils/jobs.ts
cat packages/ui/src/utils/jobs.ts
Length of output: 565
packages/ui/src/components/bundle-assets/bundle-assets.utils.ts (5)
1-20: LGTM! Well-organized imports with proper type declarations
The imports are well-structured with clear separation between lodash utilities, types, and constants.
56-84: LGTM! Well-implemented status determination logic
The function properly handles all edge cases and maintains type safety throughout the implementation.
89-127: Skip comment as it's covered in past reviews
A previous review already suggested improvements for this function's type safety and complexity.
129-140: LGTM! Clear type definitions and sorting logic
The type definitions are well-structured and the sorting logic provides a consistent ordering of assets.
218-260: Skip comment as it's covered in past reviews
A previous review already suggested improvements for breaking down the filter logic and improving performance.
packages/ui/src/components/bundle-assets/bundle-assets.tsx (5)
40-49: Great improvement in type safety!
The addition of well-defined TypeScript interfaces ViewMetricsTreemapProps and BundleAssetsProps significantly improves type safety and code maintainability. The interfaces are comprehensive and properly typed.
Also applies to: 112-130
33-33:
Fix broken import from removed file
The import statement references getFilters from bundle-assets.utils, but according to the PR summary, this file has been removed. This will cause the build to fail.
This issue was previously flagged. The getFilters function needs to be reimplemented or moved to a new location.
158-160:
Add missing dependency 'chunks' in useMemo hook
The useMemo hook depends on chunks but it's missing from the dependency array.
This issue was previously flagged. Apply this diff to fix the dependency array:
const filterFieldsData = useMemo(
() => getFilters({ compareMode: jobs?.length > 1, filters, chunks }),
- [jobs, filters, chunks],
+ [jobs, filters, chunks],
);231-237:
Include exportDialog in handleExportClick dependencies
The handleExportClick function uses exportDialog.toggle, but exportDialog is not included in the dependencies array.
This issue was previously flagged. Apply this diff to fix the dependency array:
const handleExportClick = useCallback(
(sourceType: string) => {
setExportSourceType(sourceType);
exportDialog.toggle();
},
- [exportDialog, setExportSourceType],
+ [exportDialog, setExportSourceType],
);268-268: Verify filters implementation after utils file removal
The filterFieldsData is being passed to the Filters component, but its source (getFilters) needs to be verified after the removal of the utils file.
Let's verify the implementation of filters across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/ui/src/components/asset-info/asset-info.tsx (2)
173-173: Fix typo in tooltip textThere's a typo in the tooltip text: "witch" should be "which".
- tooltip="The bundler's chunk from witch the assets was originated" + tooltip="The bundler's chunk from which the assets was originated"
141-144: Consider performance optimization and simplificationThe chunk finding and filtering logic could be optimized:
- Move the calculations into a useMemo to prevent unnecessary recalculations
- Simplify the chunkIds extraction
- const currentChunk = chunks?.find( - (chunk) => currentRun?.chunkId && currentRun.chunkId === chunk.id, - ); - const chunkIds: Array<string> = chunks?.filter(Boolean).map((chunk) => chunk.id) || []; + const { currentChunk, chunkIds } = useMemo(() => ({ + currentChunk: chunks?.find( + (chunk) => currentRun?.chunkId && currentRun.chunkId === chunk.id, + ), + chunkIds: chunks?.map((chunk) => chunk.id) ?? [], + }), [chunks, currentRun?.chunkId]);packages/utils/src/utils/component-links.ts (1)
220-235: Add parameter validation and improve documentationWhile the implementation follows the established patterns, consider adding:
- Input validation for the
chunkIdsarray andchunkId- JSDoc documentation explaining:
- The purpose of the function
- The relationship between
chunkIdsandchunkId- Expected behavior when
chunkIdis not inchunkIds+/** + * Creates a ComponentLink for filtering bundle assets by chunk + * @param chunkIds - Array of all available chunk IDs + * @param chunkId - The specific chunk ID to filter by + * @returns ComponentLink configured with chunk filters + * @throws {Error} When chunkIds is empty or chunkId is not valid + */ export const getBundleAssetsByChunk = ( chunkIds: Array<string>, chunkId: string, ): ComponentLink => ({ + if (chunkIds.length === 0) { + throw new Error('chunkIds array cannot be empty'); + } + if (!chunkIds.includes(chunkId)) { + throw new Error(`chunkId ${chunkId} not found in chunkIds`); + } section: SECTIONS.ASSETS, title: I18N.COMPONENT_LINK_BUNDLE_ASSETS_CHUNKS, params: {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/ui/src/components/asset-info/asset-info.tsx(5 hunks)packages/utils/src/i18n.js(1 hunks)packages/utils/src/utils/component-links.ts(2 hunks)
🔇 Additional comments (7)
packages/utils/src/i18n.js (1)
9-9: LGTM! Verify string usage across components.
The new i18n string follows the established naming pattern and complements existing chunk-related entries.
Let's verify the string usage:
✅ Verification successful
String constant is properly integrated and used
The i18n string is correctly used in packages/utils/src/utils/component-links.ts as part of the getBundleAssetsByChunk function, where it serves as the title for the component link that displays chunk assets. The implementation follows the established patterns and is properly integrated with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where the new i18n string is being used
# Expected: Should find references in component files using this string for chunk asset views
# Search for direct string constant usage
rg "COMPONENT_LINK_BUNDLE_ASSETS_CHUNKS" --type js --type ts --type tsx
# Search for potential chunk asset related UI components that might need this string
ast-grep --pattern 'render() {
$$$
chunks
$$$
}'
Length of output: 156
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# Search for the constant usage with ripgrep
rg "COMPONENT_LINK_BUNDLE_ASSETS_CHUNKS"
# Search for files containing "chunks" and "assets" to find related components
rg -l "chunks.*assets|assets.*chunks"
# Look for i18n imports to find potential consumers
rg -l "from ['\"]\.\./utils/i18n|from ['\"]\.\./i18n"
Length of output: 505
Script:
#!/bin/bash
# Let's check the implementation in component-links.ts to understand the usage context
# View the implementation around the i18n string usage
rg -B 5 -A 5 "COMPONENT_LINK_BUNDLE_ASSETS_CHUNKS" packages/utils/src/utils/component-links.ts
# Check if there are any tests for this functionality
fd "component-links.test" --type f
Length of output: 468
packages/ui/src/components/asset-info/asset-info.tsx (4)
5-12: Type definitions look good!
The updated type definitions and imports provide better type safety and clearer interfaces. The changes from complex object structures to more specific types (WebpackChunk, ReportMetricAssetRow) improve type checking and make the code more maintainable.
Also applies to: 24-25, 70-71
31-32: Props handling looks good!
The simplified chunk handling makes the component more maintainable and reduces complexity. The getBundleModulesByChunk usage is correct with the new parameter structure.
Also applies to: 41-41
170-183: UI implementation looks good!
The chunk-related UI section is well-structured with:
- Appropriate conditional rendering
- Consistent component reuse
- Clear separation of concerns
Line range hint 148-148: Verify type compatibility
The type assertion as any on the EntryInfo item prop suggests a potential type mismatch. Consider updating the EntryInfo component's type definitions to match the expected types.
packages/utils/src/utils/component-links.ts (2)
19-19: LGTM: Import addition is consistent with the new functionality
The addition of ASSET_CHUNK to the imports aligns with its usage in the new getBundleAssetsByChunk function.
220-235: Verify complete migration of chunk filtering logic
Let's ensure all chunk filtering functionality has been properly migrated from the removed bundle-assets.utils.js.
✅ Verification successful
Based on the results, I can now generate the final response:
Migration of chunk filtering logic appears complete and properly used
The verification shows:
- All references to
bundle-assets.utilsare in test files or component files, not in utility files - The new
getBundleAssetsByChunkfunction is properly used with correct typing inasset-info.tsx - Chunk filtering patterns are consistently implemented across the codebase with no orphaned references to old implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to old chunk filtering logic
# and verify proper usage of the new function
# Check for any remaining references to the old file
rg -l "bundle-assets.utils"
# Look for any chunk filtering patterns that might not have been migrated
rg -i "chunk.*filter|filter.*chunk" --type ts --type js
# Check for proper usage of the new function
ast-grep --pattern 'getBundleAssetsByChunk($_, $_)'
Length of output: 7595
rel: #4836
Summary by CodeRabbit
New Features
ASSET_CHUNKfor asset management.useSearchParamshook to acceptparentFilters, improving filter management.getBundleAssetsByChunkfor retrieving bundle assets filtered by chunk identifiers.Bug Fixes
Documentation
BundleAssetscomponent to improve type safety and structure.Refactor
Tests
addMetricReportAssetRowDatafunction.