Asset meta tag changes #4816
Asset meta tag changes #4816
Conversation
|
Warning Rate limit exceeded@vio has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request encompass several modifications across multiple files. Key updates include restructuring asset paths in JSON files to include an Changes
Poem
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 (
|
#11634 Bundle Size — 381.03KiB (+0.46%).43d68e5(current) vs 262308a master#11628(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #11634 |
Baseline #11628 |
|
|---|---|---|
 Initial JS |
334.13KiB(+0.32%) |
333.07KiB |
| Initial CSS |
46.91KiB(+1.49%) |
46.22KiB |
 Cache Invalidation |
40.63% |
28.42% |
 Chunks |
3 |
3 |
| Assets |
4 |
4 |
| Modules |
699(+0.87%) |
693 |
| Duplicate Modules |
0 |
0 |
| Duplicate Code |
0% |
0% |
| Packages |
39 |
39 |
| Duplicate Packages |
1 |
1 |
Bundle size by type 2 changes 2 regressions
| Current #11634 |
Baseline #11628 |
|
|---|---|---|
| JS |
334.13KiB (+0.32%) |
333.07KiB |
| CSS |
46.91KiB (+1.49%) |
46.22KiB |
Bundle analysis report Branch asset-meta-tag-changes 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: 4
🧹 Outside diff range and nitpick comments (35)
packages/ui/src/stories/wrapper.jsx (1)
6-25: Consider converting to TypeScript for better type safety.Since the PR includes TypeScript conversions for other story files, consider converting this wrapper to TypeScript as well. This would allow proper typing for
customWrapperStylesand theStorycomponent.-// wrapper.jsx +// wrapper.tsx + +interface WrapperStyles { + width?: string; + height?: string; + padding?: string; + [key: string]: any; +} + +interface StoryComponent { + (): JSX.Element; +} + export const getWrapperDecorator = - (customWrapperStyles = {}) => - (Story) => { + (customWrapperStyles: WrapperStyles = {}) => + (Story: StoryComponent) => {packages/ui/src/ui/tag/tag.tsx (2)
Line range hint
14-26: Consider adding JSDoc documentation.While the implementation is clean and follows best practices, adding JSDoc documentation would improve developer experience by providing:
- Component description
- Props documentation
- Usage examples
Add documentation like this:
+/** + * A flexible tag component that can be rendered as any HTML element + * @example + * <Tag kind={KIND.PRIMARY} size={SIZE.SMALL}>Label</Tag> + */ export const Tag = (props: TagProps) => {
Line range hint
28-29: Document static properties in JSDoc.The
KINDandSIZESstatic properties are useful for TypeScript autocompletion. Consider documenting them in the component's JSDoc to make them more discoverable.Add to the previous JSDoc suggestion:
/** * A flexible tag component that can be rendered as any HTML element * @example * <Tag kind={KIND.PRIMARY} size={SIZE.SMALL}>Label</Tag> + * + * @property {typeof KIND} KINDS - Available tag kinds + * @property {typeof SIZE} SIZES - Available tag sizes */packages/ui/src/components/bundle-assets/__tests__/get-asset-meta-status.ts (1)
3-21: Consider adding test cases for edge scenarios.While the current test cases cover the main scenarios well, consider adding tests for the following edge cases:
- Empty array input
- Arrays with more than 2 elements (to verify behavior with multiple runs)
- Non-boolean values in the array
- Undefined/null values
Here's a suggested implementation:
test('should handle edge cases appropriately', () => { // Empty array expect(getAssetMetaStatus([])).toBeFalsy(); // More than 2 elements expect(getAssetMetaStatus([true, false, true])).toEqual('added'); expect(getAssetMetaStatus([false, true, false])).toEqual('removed'); // Non-boolean values expect(() => getAssetMetaStatus([null, true])).toThrow(); expect(() => getAssetMetaStatus([undefined, false])).toThrow(); });packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (1)
9-12: Consider improving type safety and documentation.While the type definition is generally good, consider these improvements:
- The
statusprop allows bothReportMetricAssetRowMetaStatusandboolean, which could lead to inconsistent usage.- The props could benefit from JSDoc documentation explaining the purpose of each prop and valid values.
Consider this enhancement:
+/** + * Props for the AssetMetaTag component + * @property {('entry'|'initial'|'chunk')} tag - The type of asset + * @property {ReportMetricAssetRowMetaStatus} status - The current status of the asset + */ export type AssetMetaTagProps = { tag: 'entry' | 'initial' | 'chunk'; - status: ReportMetricAssetRowMetaStatus | boolean; + status: ReportMetricAssetRowMetaStatus; } & TagProps;packages/ui/src/app/app-gladys.stories.tsx (2)
24-32: Consider adding component descriptionThe meta configuration is well-structured and type-safe. However, it would be beneficial to add a description of the component's purpose and usage in the parameters.
Consider adding component documentation:
const meta: Meta<typeof App> = { title: 'App/Gladys', component: App, parameters: { layout: 'fullscreen', + docs: { + description: { + component: 'App component for Gladys integration, displaying bundle analysis results.' + } + } }, args: { version: '1.0', }, };
39-41: Consider documenting available argsThe story implementation is correct, but it would be helpful to document the available args for better discoverability.
Consider adding args documentation:
export const Default: Story = { + args: { + version: '1.0' // Document the purpose of this arg + }, + parameters: { + docs: { + description: { + story: 'Default view of the Gladys App component with bundle analysis results.' + } + } + }, render: (args) => <App jobs={JOBS} {...args} />, };packages/ui/src/app/app-outline.stories.tsx (1)
Line range hint
6-8: Consider improving fixture imports structure.The deep relative paths (../../../../) to fixtures could be simplified by either:
- Moving fixtures closer to the tests
- Creating an alias path for fixtures in the build configuration
This would improve maintainability and reduce the likelihood of path-related issues during refactoring.
packages/ui/src/app/app-home-assistant.stories.tsx (1)
2-2: LGTM! Consider adding arg descriptions.The TypeScript types and meta configuration are well-structured. The
Meta<typeof App>typing ensures proper type safety.Consider adding descriptions for the args to improve the Storybook documentation:
args: { version: '1.0', + // @type {string} The version number to display in the app },Also applies to: 24-32
packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css (2)
1-21: Consider accessibility and layout improvements.A few suggestions to enhance the root styles:
- The font size of 8px in the pseudo-element might be too small for accessibility. Consider using a relative unit (em/rem) with a minimum of 12px.
- Remove duplicate
vertical-alignproperty (appears in both.rootand::before).- Consider using
box-sizing: border-boxto prevent potential overflow issues with the pseudo-element's padding..root { --background-color: var(--color-info); background: var(--background-color); overflow: hidden; position: relative; - vertical-align: middle; + box-sizing: border-box; } .root::before { display: block; content: ' '; padding: 2px; position: absolute; left: 0; top: 0; width: 100%; height: 100%; - font-size: 8px; + font-size: 0.75rem; /* 12px at default root font size */ line-height: 1; vertical-align: middle; }
48-55: Improve maintainability and contrast for status indicators.Consider these enhancements for the status styles:
- Extract gradient angles to CSS custom properties for easier maintenance.
- Add a high contrast mode using CSS custom properties.
+:root { + --gradient-angle-added: -45deg; + --gradient-angle-removed: 135deg; +} .added { - background: linear-gradient(-45deg, var(--color-danger) 50%, var(--background-color) 50%); + background: linear-gradient( + var(--gradient-angle-added), + var(--color-danger) 50%, + var(--background-color) 50% + ); } .removed { - background: linear-gradient(135deg, var(--color-danger) 50%, var(--background-color) 50%); + background: linear-gradient( + var(--gradient-angle-removed), + var(--color-danger) 50%, + var(--background-color) 50% + ); } +@media (prefers-contrast: high) { + .added, + .removed { + outline: 2px solid var(--color-danger); + } +}packages/ui/src/components/asset-not-predictive/asset-not-predictive.tsx (1)
19-23: Consider making text content configurable.While the layout changes improve the visual hierarchy, consider making the heading and description text configurable through props. This would enhance component reusability across different contexts.
Here's a suggested implementation:
export type AssetNotPredictiveProps = { runs?: Array<Asset>; labels?: Array<string>; + heading?: string; + description?: string; } & ComponentProps<'div'>; export const AssetNotPredictive = (props: AssetNotPredictiveProps) => { - const { className = '', runs = null, labels = null, ...restProps } = props; + const { + className = '', + runs = null, + labels = null, + heading = 'Asset hash is not predictive', + description = 'File names are equal, but the size is different.', + ...restProps + } = props; return ( <Stack space="small" {...restProps} className={cx(css.root, className)}> <Stack space="xxxsmall"> - <h3>Asset hash is not predictive</h3> - <p className={css.text}>File names are equal, but the size is different.</p> + <h3>{heading}</h3> + <p className={css.text}>{description}</p> </Stack>packages/ui/src/types.d.ts (2)
22-23: Consider enhancing the meta status typeThe type could be improved in the following ways:
- Consider adding a 'modified' status for tracking changes
- Add JSDoc comments explaining the purpose and usage of this type
+/** + * Represents the change status of an asset's metadata between runs + * Used to track additions and removals of assets in the bundle + */ export type ReportMetricAssetRowMetaStatus = 'added' | 'removed';
24-49: Fix documentation inconsistenciesThe type definition has the following documentation issues:
- Duplicate documentation for
isEntry(lines 26 and 46)- Missing documentation for
fileTypepropertyexport type ReportMetricAssetRow = { /** * Asset isEntry - at least one run has isEntry truthy */ isEntry: ReportMetricAssetRowMetaStatus | boolean; /** * Asset isInitial - at least one run has isInitial truthy */ isInitial: ReportMetricAssetRowMetaStatus | boolean; /** * Asset isChunk - at least one run has isChunk truthy */ isChunk: ReportMetricAssetRowMetaStatus | boolean; /** * Asset isAsset - at least one run has isAsset truthy */ isAsset: boolean; /** * Asset name is not predictive */ isNotPredictive: boolean; - /** - * Report asset row isEntry - at least one run has isEntry - */ + /** + * Type of the asset file (e.g., js, css, img) + */ fileType: string; } & Omit<ReportMetricRow, 'runs'> & { runs: Array<(Asset & ReportMetricRun) | null> };Additionally, consider adding a type for the file type values to ensure consistency:
export type AssetFileType = 'js' | 'css' | 'img' | 'font' | 'other';Then update the
fileTypeproperty:- fileType: string; + fileType: AssetFileType;packages/ui/src/components/asset-name/asset-name.stories.tsx (3)
5-20: Add type safety and improve readability.Consider the following improvements:
- Add TypeScript interface/type for the row structure
- Extract magic numbers into named constants
+interface AssetRow { + key: string; + label: string; + changed: boolean; + isEntry: boolean | 'added' | 'removed'; + isChunk: boolean | 'added' | 'removed'; + isInitial: boolean | 'added' | 'removed'; + isAsset: boolean; + isNotPredictive: boolean; + fileType: string; + biggerIsBetter: boolean; + runs: Array<{ name: string; value: number }>; +} +const KB = 1024; +const SAMPLE_SIZES = { + baseline: 10 * KB, + current: 11 * KB, +}; -const ROW = { +const ROW: AssetRow = { key: 'asset/main.js', label: 'asset/main.js', // ... other properties runs: [ - { name: 'asset/main.abcd1234.js', value: 1024 * 10 }, - { name: 'asset/main.abcd1234.js', value: 1024 * 11 }, + { name: 'asset/main.abcd1234.js', value: SAMPLE_SIZES.baseline }, + { name: 'asset/main.abcd1234.js', value: SAMPLE_SIZES.current }, ], };
22-32: Enhance type safety for default args.The meta configuration looks good, but consider adding type safety for the default args.
+interface DefaultArgs { + customComponentLink: React.ElementType; + filters: Record<string, unknown>; + search: string; +} const meta = { title: 'Components/AssetName', component: AssetName, - args: { + args: { customComponentLink: 'span', filters: {}, search: '', - }, + } satisfies DefaultArgs, } as Meta<typeof AssetName>;
34-85: Consider improving type consistency and reducing duplication.The stories effectively demonstrate different states, but there are opportunities for improvement:
- The type of meta flags varies between boolean and string ('added'/'removed'), which could lead to confusion
- There's repeated code in story configurations
Consider this refactor to reduce duplication and improve type safety:
+type MetaStatus = boolean | 'added' | 'removed'; +const createMetaRow = (metaStatus: MetaStatus) => ({ + ...ROW, + isEntry: metaStatus, + isChunk: metaStatus, + isInitial: metaStatus, + isAsset: false, +}); export const MetaDefault: Story = { args: { - row: { - ...ROW, - isEntry: true, - isChunk: true, - isInitial: true, - isAsset: false, - }, + row: createMetaRow(true), }, }; export const MetaAdded: Story = { args: { - row: { - ...ROW, - isEntry: 'added', - isChunk: 'added', - isInitial: 'added', - isAsset: false, - }, + row: createMetaRow('added'), }, };packages/ui/src/components/asset-name/asset-name.tsx (3)
17-21: Add JSDoc comments to document prop types.Consider adding documentation to explain the purpose of each prop, especially
customComponentLink.+/** + * Props for the AssetName component + */ export type AssetNameProps = { + /** Optional CSS class name for styling */ className?: string; + /** Asset row data containing metadata and display information */ row: ReportMetricAssetRow; + /** Custom component to use for rendering the asset link */ customComponentLink: ElementType; };
23-26: Optimize props destructuring.Consider destructuring nested properties in a single step to improve readability.
- const { className = '', customComponentLink: CustomComponentLink, row } = props; - const { label, isNotPredictive, runs, isChunk, isEntry, isInitial } = row; + const { + className = '', + customComponentLink: CustomComponentLink, + row: { label, isNotPredictive, runs, isChunk, isEntry, isInitial, key } + } = props;
29-37: Add error boundary for AssetNotPredictive component.The
AssetNotPredictivecomponent might fail ifrunsdata is malformed. Consider wrapping it in an error boundary.packages/ui/src/components/bundle-assets/index.jsx (1)
48-49: LGTM! Consider adding TypeScript type safety.The simplified asset processing logic using
addMetricReportAssetRowDataimproves code clarity. Since this is a critical data processing step, consider converting this file to TypeScript for better type safety and maintainability.Apply this change to improve type safety:
- const result = webpack.compareBySection.assets(jobs, [addMetricReportAssetRowData]); + const result: Array<AssetMetricRow> = webpack.compareBySection.assets(jobs, [addMetricReportAssetRowData]);Note: You'll need to define the
AssetMetricRowtype or import it from the appropriate module.packages/ui/src/components/bundle-assets/__tests__/get-is-not-predictive.ts (2)
1-3: Consider adding test framework type imports for better type safety.Since this is a TypeScript file, consider importing the test framework types for better type safety and IDE support.
+import type { describe, test, expect } from '@jest/globals'; import { getIsNotPredictive } from '../bundle-assets.utils';
4-91: Improve test maintainability and readability.Consider the following improvements:
- Extract repeated test data into fixtures or factory functions
- Define constants for magic numbers
- Split into separate test cases with specific descriptions
Example refactor:
const createAssetRun = (name: string, value: number) => ({ name, value, isEntry: true, isChunk: true, isInitial: true, }); const KB = 1024; const createTestRow = (runs: Array<AssetRun | null>) => ({ key: 'assets/main.js', label: 'assets/main.js', biggerIsBetter: false, changed: false, runs, }); test('should return false for single entry', () => { expect( getIsNotPredictive( createTestRow([createAssetRun('assets/main.abc123.js', 10 * KB)]) ) ).toBeFalsy(); }); test('should return false when second entry is null', () => { // ... similar pattern for other cases });packages/ui/src/app/app.stories.tsx (1)
80-111: Consider enhancing the AssetMetaChanges story.The current implementation could be improved in several ways:
- The story only demonstrates changes in the
initialflag- There's duplicate code when setting up current and baseline data
Consider this refactoring to reduce duplication and enhance test coverage:
export const AssetMetaChanges: Story = { render: (args) => { - const current = merge({}, CURRENT_SOURCE); - const baseline = merge({}, CURRENT_SOURCE); + const createAssetData = (isInitial: boolean) => ({ + webpack: { + ...CURRENT_SOURCE.webpack, + assets: [ + ...CURRENT_SOURCE.webpack.assets, + { + name: 'assets/js/vendors-auth.1a278dc.js', + size: 26364, + } + ], + chunks: [ + ...CURRENT_SOURCE.webpack.chunks, + { + id: 100, + entry: false, + initial: isInitial, + files: ['assets/js/vendors-auth.1a278dc.js'], + names: ['vendors-auth'], + } + ], + }, + }); - current.webpack.assets.push({ - name: 'assets/js/vendors-auth.1a278dc.js', - size: 26364, - }); - current.webpack.chunks.push({ - id: 100, - entry: false, - initial: false, - files: ['assets/js/vendors-auth.1a278dc.js'], - names: ['vendors-auth'], - }); - - baseline.webpack.assets.push({ - name: 'assets/js/vendors-auth.1a278dc.js', - size: 26364, - }); - baseline.webpack.chunks.push({ - id: 100, - entry: false, - initial: true, - files: ['assets/js/vendors-auth.1a278dc.js'], - names: ['vendors-auth'], - }); + const current = createAssetData(false); + const baseline = createAssetData(true); return <App jobs={createJobs([current, baseline])} {...args} />; }, };Consider adding more variations of metadata changes to improve test coverage, such as:
- Changes in asset size
- Changes in chunk names
- Changes in entry points
packages/ui/src/components/asset-info/asset-info.tsx (1)
92-128: Consider extracting repeated tag patterns.The implementation correctly replaces
TagwithAssetMetaTag, but there's repeated code structure across entry, initial, and chunk tags. Consider extracting this pattern into a reusable helper function.const renderAssetTag = ( type: 'entry' | 'initial' | 'chunk', isEnabled: boolean, CustomComponentLink: React.ElementType, onClick: () => void, ) => { const labels = { entry: 'Entrypoint', initial: 'Initial', chunk: 'Chunk', }; return isEnabled && ( <AssetMetaTag as={CustomComponentLink} {...getBundleAssetsByEntryType(type)} onClick={onClick} tag={type} size="medium" status={isEnabled} className={cx( css.assetNameTag, type !== 'entry' && css[`assetNameTag${type.charAt(0).toUpperCase()}${type.slice(1)}`], )} > {labels[type]} </AssetMetaTag> ); };packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts (4)
4-42: Consider improving test readability and maintainability.While the test case is thorough, consider these improvements:
- Extract the magic number
1024 * 10into a named constant (e.g.,ASSET_SIZE_BYTES)- Consider extracting common test data into fixtures to reduce duplication across tests
- Make the test description more specific, e.g., "should add metadata and flags to a single run asset"
+const ASSET_SIZE_BYTES = 1024 * 10; +const createTestAsset = (overrides = {}) => ({ + name: 'assets/main.abc123.js', + value: ASSET_SIZE_BYTES, + isEntry: true, + isChunk: true, + isInitial: true, + ...overrides, +}); -test('should add data to a single run', () => { +test('should add metadata and flags to a single run asset', () => { expect( addMetricReportAssetRowData({ key: 'assets/main.js', label: 'assets/main.js', biggerIsBetter: false, changed: false, - runs: [ - { - name: 'assets/main.abc123.js', - value: 1024 * 10, - isEntry: true, - isChunk: true, - isInitial: true, - }, - ], + runs: [createTestAsset()], }), ).toEqual({ // ... rest of the test remains same }); });
44-84: Fix typo in test name and reduce data duplication.
- The test name contains a duplicate word "add":
should add add data- The test data structure is duplicated from the first test
-test('should add add data when baseline run is null', () => { +test('should add data when baseline run is null', () => { expect( addMetricReportAssetRowData({ key: 'assets/main.js', label: 'assets/main.js', biggerIsBetter: false, changed: false, - runs: [ - { - name: 'assets/main.abc123.js', - value: 1024 * 10, - isEntry: true, - isChunk: true, - isInitial: true, - }, - null, - ], + runs: [createTestAsset(), null], }), ).toEqual({ // ... rest of the test remains same }); });
86-126: Fix typo and highlight changed flag testing.
- The test name contains a duplicate word "add":
should add add data- This test case appropriately verifies the
changed: trueflag behavior with a null current run-test('should add add data when curent run is null', () => { +test('should add data when current run is null', () => { expect( addMetricReportAssetRowData({ key: 'assets/main.js', label: 'assets/main.js', biggerIsBetter: false, changed: true, - runs: [ - null, - { - name: 'assets/main.abc123.js', - value: 1024 * 10, - isEntry: true, - isChunk: true, - isInitial: true, - }, - ], + runs: [null, createTestAsset()], }), ).toEqual({ // ... rest of the test remains same }); });
128-181: Consider additional test cases for baseline comparison.While this test covers the unchanged state between runs, consider adding test cases for:
- Different values between runs
- Different asset names between runs
- Different flags (isEntry, isChunk, isInitial) between runs
This would provide better coverage for the comparison logic.
test('should add data when baseline is not null', () => { expect( addMetricReportAssetRowData({ key: 'assets/main.js', label: 'assets/main.js', biggerIsBetter: false, changed: false, - runs: [ - { - name: 'assets/main.abc123.js', - value: 1024 * 10, - isEntry: true, - isChunk: true, - isInitial: true, - }, - { - name: 'assets/main.abc123.js', - value: 1024 * 10, - isEntry: true, - isChunk: true, - isInitial: true, - }, - ], + runs: [createTestAsset(), createTestAsset()], }), ).toEqual({ // ... rest of the test remains same }); }); +test('should handle different values between runs', () => { + expect( + addMetricReportAssetRowData({ + key: 'assets/main.js', + label: 'assets/main.js', + biggerIsBetter: false, + changed: true, + runs: [ + createTestAsset({ value: ASSET_SIZE_BYTES }), + createTestAsset({ value: ASSET_SIZE_BYTES * 2 }), + ], + }), + ).toEqual({ + // Add expected result + }); +});packages/ui/src/components/bundle-assets/bundle-assets.jsx (2)
207-216: Consider explicit prop forwarding for better maintainabilityWhile the implementation is functionally correct, consider explicitly listing the props you want to forward instead of using rest spread. This makes the component contract more explicit and prevents passing unexpected props.
const assetNameCustomComponentLink = useCallback( - ({ entryId: assetEntryId, ...assetNameRestProps }) => ( + ({ entryId: assetEntryId, className, children }) => ( <CustomComponentLink section={SECTIONS.ASSETS} params={{ [COMPONENT.BUNDLE_ASSETS]: { filters, search, entryId: assetEntryId } }} - {...assetNameRestProps} + className={className} + children={children} /> ), [CustomComponentLink, filters, search], );
Line range hint
217-227: Optimize useCallback dependenciesThe dependency array includes indirect dependencies. Since the function only directly depends on
assetNameCustomComponentLink, the other dependencies should be removed to prevent unnecessary re-renders.const renderRowHeader = useCallback( (row) => ( <AssetName row={row} customComponentLink={assetNameCustomComponentLink} className={css.assetName} /> ), - [CustomComponentLink, filters, search], + [assetNameCustomComponentLink], );packages/ui/src/components/bundle-assets/bundle-assets.utils.js (4)
1-5: Use@typedefinstead of@typefor type definitionsThe JSDoc comments are using
@typeto define type aliases. To accurately define new types or type aliases,@typedefshould be used instead. This change adheres to JSDoc conventions and enhances code clarity.Apply this diff to correct the JSDoc comments:
/** -- * @type {import('@bundle-stats/utils').ReportMetricRow} ReportMetricRow -- * @type {import('../../types').ReportMetricAssetRow} ReportMetricAssetRow -- * @type {import('../../types').ReportMetricAssetRowMetaStatus} ReportMetricAssetRowFlagStatus ++ * @typedef {import('@bundle-stats/utils').ReportMetricRow} ReportMetricRow ++ * @typedef {import('../../types').ReportMetricAssetRow} ReportMetricAssetRow ++ * @typedef {import('../../types').ReportMetricAssetRowMetaStatus} ReportMetricAssetRowFlagStatus */
42-44: Add missing parameter name in JSDoc commentThe
@paramtag in the JSDoc comment forgetAssetMetaStatusis missing the parameter namevalues. Including the parameter name improves documentation and code readability.Apply this diff to fix the JSDoc comment:
/** -- * @param {Array<boolean>} ++ * @param {Array<boolean>} values * @returns {ReportMetricAssetRowFlagStatus | boolean} */
83-91: Simplify array construction usingmapInstead of initializing empty arrays and populating them using
forEach, you can use themapmethod to create the arrays more concisely and functionally.Apply this diff to refactor the code:
// Collect meta for each run - const runsEntry = []; - const runsInitial = []; - const runsChunk = []; - - runs.forEach((run) => { - runsEntry.push(run?.isEntry); - runsInitial.push(run?.isInitial); - runsChunk.push(run?.isChunk); - }); + const runsEntry = runs.map((run) => run?.isEntry); + const runsInitial = runs.map((run) => run?.isInitial); + const runsChunk = runs.map((run) => run?.isChunk);
Line range hint
18-39: Use descriptive variable names ingetIsNotPredictiveConsider renaming the accumulator variable
aggto a more descriptive name likeisNotPredictiveto enhance code readability and clarify the purpose of the variable.Apply this diff to rename the variable:
return runs.reduce((agg, current, index) => { - if (agg) { - return agg; + if (isNotPredictive) { + return isNotPredictive; } // ... rest of the code remains unchanged ... - return agg; + return isNotPredictive; }, false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
fixtures/webpack-stats.baseline.json(1 hunks)fixtures/webpack-stats.current.json(1 hunks)packages/ui/src/app/app-gladys.stories.tsx(2 hunks)packages/ui/src/app/app-home-assistant.stories.tsx(2 hunks)packages/ui/src/app/app-outline.stories.tsx(2 hunks)packages/ui/src/app/app.stories.jsx(0 hunks)packages/ui/src/app/app.stories.tsx(1 hunks)packages/ui/src/components/asset-info/asset-info.tsx(2 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css(1 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx(1 hunks)packages/ui/src/components/asset-meta-tag/index.ts(1 hunks)packages/ui/src/components/asset-name/asset-name.module.css(1 hunks)packages/ui/src/components/asset-name/asset-name.stories.tsx(1 hunks)packages/ui/src/components/asset-name/asset-name.tsx(1 hunks)packages/ui/src/components/asset-name/index.ts(1 hunks)packages/ui/src/components/asset-not-predictive/asset-not-predictive.tsx(2 hunks)packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts(1 hunks)packages/ui/src/components/bundle-assets/__tests__/get-asset-meta-status.ts(1 hunks)packages/ui/src/components/bundle-assets/__tests__/get-is-not-predictive.ts(1 hunks)packages/ui/src/components/bundle-assets/bundle-assets.jsx(2 hunks)packages/ui/src/components/bundle-assets/bundle-assets.module.css(0 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(2 hunks)packages/ui/src/components/bundle-assets/index.jsx(2 hunks)packages/ui/src/components/entry-info/entry-info.tsx(1 hunks)packages/ui/src/components/metrics-treemap/metrics-treemap.utils.ts(0 hunks)packages/ui/src/stories/wrapper.jsx(1 hunks)packages/ui/src/types.d.ts(2 hunks)packages/ui/src/ui/tag/tag.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- packages/ui/src/app/app.stories.jsx
- packages/ui/src/components/bundle-assets/bundle-assets.module.css
- packages/ui/src/components/metrics-treemap/metrics-treemap.utils.ts
✅ Files skipped from review due to trivial changes (3)
- fixtures/webpack-stats.current.json
- packages/ui/src/components/asset-meta-tag/index.ts
- packages/ui/src/components/asset-name/index.ts
🧰 Additional context used
🪛 Biome
packages/ui/src/components/asset-name/asset-name.module.css
[error] 1-1: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 10-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (31)
packages/ui/src/stories/wrapper.jsx (1)
4-4: LGTM! QueryStateProvider integration looks good.
The addition of QueryStateProvider aligns with the PR objectives and provides a consistent state management context for all stories.
Also applies to: 18-22
packages/ui/src/ui/tag/tag.tsx (2)
1-12: Well-structured type definition with good practices!
The conversion to a type alias and extension of ComponentProps<'span'> improves type safety and component flexibility. The polymorphic as prop is a great addition for component composition.
8-12: Verify type changes impact.
Let's verify that the type changes don't break existing usages, particularly in the AssetMetaTag component.
✅ Verification successful
Type changes are safe for existing usages
The verification shows that the Tag component is used safely across the codebase:
- AssetMetaTag properly extends TagProps and uses standard Tag properties (size, kind)
- Other components use Tag with supported props:
kind="danger"in package-info and module-infosize="small"in bundle-modules- Basic usage with className in jobs-header
All usages align with the defined TagProps type which includes:
- Optional
asprop for element type - Optional
kindandsizeprops - Span component props through ComponentProps<'span'>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Tag component usage to verify type compatibility
echo "Searching for Tag component imports and usage..."
rg -l "import.*Tag.*from.*tag'" --type ts --type tsx
echo "\nChecking Tag component usage patterns..."
rg "Tag" -A 3 --type ts --type tsx
Length of output: 350
Script:
#!/bin/bash
# Search for Tag component imports and usage with correct file type flags
echo "Searching for Tag component imports..."
rg -t typescript -t typescriptreact "import.*Tag.*from.*tag"
echo "\nChecking Tag component usage patterns..."
rg -t typescript -t typescriptreact "Tag" -A 3
echo "\nSearching for files containing Tag..."
fd -e tsx -e ts | xargs rg "Tag"
Length of output: 4517
packages/ui/src/components/bundle-assets/__tests__/get-asset-meta-status.ts (1)
1-21: LGTM! Well-structured test suite with clear scenarios.
The test cases are well-organized, with clear descriptions and expectations. The implementation follows good testing practices and provides good coverage for the main scenarios.
packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (3)
1-8: LGTM! Well-organized imports with proper separation of concerns.
The imports follow a clean structure with external dependencies first, followed by internal components and types, and finally styles. The use of CSS modules ensures proper style encapsulation.
14-16: LGTM! Clean prop handling and destructuring.
The component follows React best practices with proper prop destructuring and default value handling.
17-24: Consider adding validation for status values.
The className construction assumes valid status values but doesn't handle invalid ones. This could lead to unexpected styling if an invalid boolean or status value is provided.
Let's verify the possible status values:
Consider adding validation:
const rootClassName = cx(
css.root,
- status === 'added' && css.added,
- status === 'removed' && css.removed,
+ status === 'added' ? css.added : null,
+ status === 'removed' ? css.removed : null,
css[tag],
className,
);packages/ui/src/app/app-gladys.stories.tsx (2)
2-2: LGTM: Type definitions are properly implemented
The TypeScript types are correctly imported and the Story type is properly derived from the meta configuration.
Also applies to: 37-37
Line range hint 6-8: Verify test fixtures organization
The test fixtures are imported using relative paths that require ESLint rule disabling. Consider moving these fixtures to a more appropriate location within the UI package.
✅ Verification successful
Let me try another verification approach to check the fixture imports and directory structure.
Let me try one final verification to check the repository structure and existing test data organization.
Move test fixtures to the repository's dedicated fixtures directory
The repository already has a dedicated fixtures/ directory at the root level, which is the appropriate location for test data. The current relative import path (../../../../fixtures/gladys/) suggests these files already exist in the root fixtures directory. Update the imports to use the package-relative path instead:
import currentData from '@repo/fixtures/gladys/current.json';
import baselineData from '@repo/fixtures/gladys/baseline.json';
import { metaBaseline, metaCurrent } from '@repo/fixtures/meta';🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are other stories using similar fixture imports
# and identify potential locations for centralizing test data.
echo "Checking for similar fixture imports in other stories:"
rg -l "fixtures/gladys" --type ts --type tsx
echo "\nChecking for existing fixture directories in the UI package:"
fd -t d "fixtures" packages/ui/
Length of output: 358
Script:
#!/bin/bash
# Check for any fixture imports and existing fixture directories
echo "Checking for fixture imports in stories:"
rg -l "fixtures/" --type typescript
echo -e "\nChecking directory structure:"
fd . packages/ui/src -t d
echo -e "\nChecking for JSON files that might be fixtures:"
fd -e json . packages/ui/
Length of output: 3154
Script:
#!/bin/bash
# Check repository structure and test data organization
echo "Checking repository root structure:"
fd -d 1 -t d .
echo -e "\nChecking for existing test fixtures:"
fd fixtures .
echo -e "\nChecking for story files:"
fd -e stories.tsx . packages/ui/
Length of output: 2791
packages/ui/src/app/app-outline.stories.tsx (2)
2-2: LGTM! Good TypeScript configuration.
The TypeScript types are properly imported and configured, providing better type safety and IDE support.
Also applies to: 37-37
39-41: LGTM! Well-structured story definition.
The Default story is properly typed and follows modern Storybook practices. The render function correctly composes the App component with both static and dynamic props.
packages/ui/src/app/app-home-assistant.stories.tsx (1)
37-41: Consider adding prop validation and test coverage.
The story configuration is well-structured, but consider adding validation for the version prop and ensuring test coverage for different version scenarios.
Let's verify the prop types and test coverage:
Would you like me to help create additional test cases for version prop validation?
packages/ui/src/components/asset-not-predictive/asset-not-predictive.tsx (2)
10-13: LGTM! Well-structured type definitions.
The conversion to a type alias and extension of ComponentProps<'div'> improves type safety and component reusability.
15-17: LGTM! Clean component implementation.
The component follows React best practices with proper props typing and default value handling.
packages/ui/src/types.d.ts (1)
2-2: LGTM: Import statement is correctly updated
The addition of the Asset type import is consistent with the new asset-related type definitions.
packages/ui/src/components/asset-name/asset-name.stories.tsx (1)
1-4: LGTM! Clean and focused imports.
The imports are well-structured, using type imports appropriately and maintaining a clean separation between external and local dependencies.
packages/ui/src/components/asset-name/asset-name.tsx (2)
1-16: LGTM! Well-organized imports and constants.
The imports follow a clear organization pattern, and the constants are properly defined for run labels.
23-60: Overall implementation looks solid!
The component successfully implements the asset name rendering with metadata support, aligning well with the PR objectives. While there are some suggested improvements above, the core functionality is well-structured and maintainable.
packages/ui/src/components/bundle-assets/index.jsx (1)
16-16: Verify the functionality coverage of the consolidated utility.
The consolidation of addRowAssetFlags and addRowIsNotPredictive into addMetricReportAssetRowData looks good from a maintainability perspective.
Let's verify that the new utility covers all previous functionality:
✅ Verification successful
Consolidated utility provides complete functionality coverage
Based on the test file examination, addMetricReportAssetRowData fully implements the required functionality:
- Handles asset flags (
isEntry,isChunk,isInitial,isAsset) - Implements predictive state handling (
isNotPredictive) - Processes both single and multiple runs
- Handles null cases for baseline and current runs
- Maintains file type detection and metadata
The consolidation successfully combines the functionality of the previous utilities addRowAssetFlags and addRowIsNotPredictive into a single, well-tested utility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new utility implements all required functionality
# Check the implementation of the new utility
ast-grep --pattern 'function addMetricReportAssetRowData($_) {
$$$
}'
# Look for any remaining references to old utilities to ensure complete migration
rg 'addRowAssetFlags|addRowIsNotPredictive'
Length of output: 119
Script:
#!/bin/bash
# Let's try a different approach to find the utility implementation
# Search for the file containing the utilities
fd "bundle-assets.utils" --type f
# Once we find the file, let's examine its content
rg -A 10 "addMetricReportAssetRowData"
# Also search for any tests that might help understand the functionality
fd "bundle-assets" --type f | grep -i "test\|spec"
Length of output: 8762
Script:
#!/bin/bash
# Let's examine the content of the test file to understand the functionality coverage
cat "packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts"
# Also check for any removed utility tests to compare functionality
rg -l "addRowAssetFlags|addRowIsNotPredictive" --glob "**/__tests__/**"
Length of output: 4369
packages/ui/src/components/bundle-assets/__tests__/get-is-not-predictive.ts (1)
1-119: Verify test coverage against implementation.
Let's verify that all cases in the implementation are covered by these tests.
✅ Verification successful
Let me try another search strategy to find the implementation.
Let me try one more search strategy to locate the implementation.
Let me check the implementation in the JavaScript utils file since we found it's .js instead of .ts.
Tests adequately cover the implementation
The test file correctly covers both the default case (returning false) and the specific case where the function returns true. Looking at the implementation, getIsNotPredictive returns true only when:
- Same asset name appears in consecutive runs
- The key is different from the asset name
- The values are different
All these conditions are verified in the test cases:
- Default false cases cover different scenarios (single run, null run, same values)
- True case verifies when same named assets have different values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and analyze the implementation file
echo "Searching for implementation file..."
rg --type ts "export function getIsNotPredictive" -A 10
# Find any additional test files
echo "Checking for other test files..."
fd -e ts -e tsx "get-is-not-predictive" packages/ui/src
Length of output: 347
Script:
#!/bin/bash
# Search for the implementation in bundle-assets.utils.ts
echo "Searching in utils file..."
rg -t ts "getIsNotPredictive" packages/ui/src/components/bundle-assets/bundle-assets.utils.ts -A 10
# If not found, try to locate the file first
echo "Locating utils file..."
fd "bundle-assets.utils.ts" packages/ui/src
Length of output: 365
Script:
#!/bin/bash
# Search for any file containing getIsNotPredictive
echo "Searching for getIsNotPredictive in all files..."
rg -t ts "export.*getIsNotPredictive" --context 10
# List all files in the bundle-assets directory
echo "Listing bundle-assets directory..."
fd . packages/ui/src/components/bundle-assets --type f
Length of output: 904
Script:
#!/bin/bash
# Check the implementation in the JavaScript file
echo "Checking bundle-assets.utils.js..."
cat packages/ui/src/components/bundle-assets/bundle-assets.utils.js
Length of output: 3305
packages/ui/src/app/app.stories.tsx (4)
1-11: LGTM! Well-structured imports with proper TypeScript types.
The imports are properly organized with appropriate type definitions and necessary eslint configurations for fixture data.
46-60: LGTM! Well-configured Storybook meta with proper typing.
The meta configuration follows Storybook best practices with proper TypeScript types and layout settings.
61-79: LGTM! Comprehensive story coverage.
The stories provide good coverage of various scenarios including default, no insights, no baseline, empty baseline, multiple baselines, and empty states.
Also applies to: 113-127
28-42: Verify the impact of filtered assets in MULTIPLE_JOBS.
The asset filtering in MULTIPLE_JOBS only includes CSS and JS files, which might not cover all relevant asset types. Additionally, limiting modules to 100 could affect test coverage.
Let's check what asset types might be missed:
✅ Verification successful
Let me try a different approach to verify this by searching for the fixture data and asset patterns in the codebase.
Let me check the fixture file content and any other related test files to understand the asset types being used in tests.
Let me try one final verification to check the webpack stats file and any test files using standard file extensions.
Based on the webpack stats data, I can now provide a definitive response about the asset filtering:
The asset filtering in MULTIPLE_JOBS is appropriate for its use case
The current filter asset.name.match(/.(css|js)$/) correctly focuses on the main bundle assets (CSS and JS) while excluding auxiliary files that don't impact bundle size analysis:
- Excluded files are primarily static assets like:
- Favicons and images (*.ico, *.png, *.svg)
- Configuration files (*.xml, *.webmanifest)
- Static HTML pages (*.html)
- Source maps (*.map)
The filtering helps maintain focus on the relevant bundle metrics without noise from static assets. Additionally, the 100 module limit is acceptable for story/example data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for asset types that might be missed by the current filter
# Expected: List of unique asset extensions to ensure we're not missing important types
echo "Checking asset types in fixture data..."
cat ../../../../fixtures/job.baseline.json | jq -r '.rawData.webpack.assets[].name' | grep -o '\.[^.]*$' | sort -u
Length of output: 276
Script:
#!/bin/bash
# First, let's locate the fixture file
fd "job.baseline" --type f
# Then let's look for any webpack asset patterns in the codebase
rg "webpack.*assets.*filter" -A 2
Length of output: 692
Script:
#!/bin/bash
# Check the fixture file content
cat fixtures/job.baseline.js
# Look for test files that might be using these assets
rg "webpack.*assets" --type test -A 2
# Look for any asset type definitions or constants
rg "asset.*type|ASSET_TYPE" --type ts --type js
Length of output: 457
Script:
#!/bin/bash
# Check the webpack stats baseline file
cat fixtures/webpack-stats.baseline.json
# Look for test files using standard extensions
rg "webpack.*assets" -g "*.test.ts" -g "*.test.tsx" -g "*.test.js" -g "*.test.jsx" -g "*.spec.ts" -g "*.spec.tsx" -g "*.spec.js" -g "*.spec.jsx" -A 2
Length of output: 239436
packages/ui/src/components/asset-info/asset-info.tsx (2)
15-15: LGTM: Clean import of the extracted component.
The import of AssetMetaTag aligns with the PR objective of extracting this component for better modularity.
97-97: Verify if the hardcoded size is intentional.
The size="medium" prop is hardcoded for all tags. Verify if this should be configurable based on context or design requirements.
packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts (2)
1-3: LGTM! Clean test suite setup.
The import statement and test suite description are clear and follow best practices.
1-181: Overall good test coverage with room for improvement.
The test file provides comprehensive coverage of the addMetricReportAssetRowData utility function's core functionality. The test cases effectively verify:
- Single run processing
- Null baseline handling
- Null current run handling
- Basic baseline comparison
To make the test suite more robust and maintainable:
- Extract common test data into fixtures
- Add test cases for different comparison scenarios
- Fix typos in test names
- Use constants for magic numbers
Despite these improvement opportunities, the current implementation is solid and ready for review.
packages/ui/src/components/entry-info/entry-info.tsx (1)
148-148: LGTM! Removal of redundant heading improves HTML semantics.
The removal of as="h3" from this FlexStack is actually an improvement. The parent component already establishes the correct heading structure with as="header" and another as="h3", so having a nested h3 here would have been semantically incorrect. This change helps maintain a proper document outline.
packages/ui/src/components/bundle-assets/bundle-assets.jsx (1)
39-39: LGTM: Clean import addition
The import of the AssetName component follows the project's import conventions and aligns with the component restructuring.
fixtures/webpack-stats.baseline.json (1)
97-100: LGTM! Asset path organization improvements.
The changes properly organize vendor assets into type-specific directories (assets/css/ and assets/js/), following common conventions for asset organization.
Let's verify that the asset references are updated consistently:
✅ Verification successful
Let me analyze the test file to understand if the references without the 'assets/' prefix are intentional.
Asset path changes are safe to proceed
The references without 'assets/' prefix found in utils-calculate-cache-invalidation.ts are test fixtures that simulate webpack stats for cache invalidation calculations. These test cases are independent of the actual asset paths in the webpack stats files and don't need to be updated since they're just mock data for testing purposes.
The actual webpack stats files (webpack-stats.baseline.json and webpack-stats.current.json) consistently use the new prefixed paths, which is the correct implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any remaining references to the old asset paths
# Test: Search for direct vendor file references without the assets prefix
rg -A 2 'vendors\.[a-f0-9]+\.(css|js|map)' --glob '!fixtures/webpack-stats.baseline.json'
Length of output: 2380
Script:
#!/bin/bash
# Get the full context of the test file to understand the test cases
cat packages/utils/src/webpack/__tests__/utils-calculate-cache-invalidation.ts
Length of output: 3702
| test('should return true when not predictive', () => { | ||
| expect( | ||
| getIsNotPredictive({ | ||
| key: 'assets/main.js', | ||
| label: 'assets/main.js', | ||
| biggerIsBetter: false, | ||
| changed: false, | ||
| runs: [ | ||
| { | ||
| name: 'assets/main.abc123.js', | ||
| value: 1024 * 10, | ||
| isEntry: true, | ||
| isChunk: true, | ||
| isInitial: true, | ||
| }, | ||
| { | ||
| name: 'assets/main.abc123.js', | ||
| value: 1024 * 11, | ||
| isEntry: true, | ||
| isChunk: true, | ||
| isInitial: true, | ||
| }, | ||
| ], | ||
| }), | ||
| ).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for non-predictive scenarios.
The current test only covers one scenario where the function returns true. Consider:
- Adding more test cases for other non-predictive scenarios
- Making the test description more specific about what makes an asset "not predictive"
- Adding a comment explaining the business logic
Example enhancement:
describe('non-predictive scenarios', () => {
// When same filename has different values across runs
test('should return true when same asset has different values', () => {
// existing test case
});
// Add more cases like:
test('should return true when chunk/entry flags differ', () => {
// test where isChunk or isEntry differ between runs
});
test('should return true when initial/async state differs', () => {
// test where isInitial differs between runs
});
});
vio
force-pushed
the
asset-meta-tag-changes
branch
from
2ebb19e
to
93d21e8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
packages/ui/src/components/asset-info/asset-info.module.css (1)
32-36: LGTM! Clean flexbox implementation.The flexbox layout with centered alignment is well-implemented and follows the design system's spacing variables.
Consider simplifying the gap to use the
--space-xxxsmallvariable directly instead of calculating half of it:.runNameTags { display: flex; align-items: center; - gap: calc(var(--space-xxxsmall) / 2); + gap: var(--space-xxxsmall); }packages/ui/src/components/asset-name/asset-name.module.css (2)
1-2: Remove empty root class if unused.The
.rootclass is currently empty and flagged by static analysis. If it's not needed for component targeting or future styling, consider removing it to maintain a cleaner codebase.-.root {} -🧰 Tools
🪛 Biome
[error] 1-1: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
10-11: Consider removing empty anchor class.The
.notPredictiveAnchorclass is empty and flagged by static analysis. If it's not required for component targeting, consider removing it.-.notPredictiveAnchor {} -🧰 Tools
🪛 Biome
[error] 10-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (2)
9-12: Consider enhancing type safety and documentation.The type definition could be improved in the following ways:
- The
statusprop allows boolean values which might lead to inconsistent usage. Consider restricting it to justReportMetricAssetRowMetaStatusif boolean values aren't necessary.- Adding JSDoc comments would improve developer experience.
+/** + * Props for the AssetMetaTag component + * @property {('entry'|'initial'|'chunk')} tag - The type of asset tag to display + * @property {ReportMetricAssetRowMetaStatus} status - The status of the asset + */ export type AssetMetaTagProps = { tag: 'entry' | 'initial' | 'chunk'; - status: ReportMetricAssetRowMetaStatus | boolean; + status: ReportMetricAssetRowMetaStatus; } & TagProps;
14-34: Consider performance optimization.Since this component might be used in lists of assets, consider memoizing it to prevent unnecessary re-renders.
-export const AssetMetaTag = (props: AssetMetaTagProps) => { +export const AssetMetaTag = React.memo((props: AssetMetaTagProps) => { const { className = '', title, tag, status, ...restProps } = props; const rootClassName = cx( css.root, status === 'added' && css.added, status === 'removed' && css.removed, css[tag], className, ); return ( <Tag className={rootClassName} title={title} size={Tag.SIZES.SMALL} kind={Tag.KINDS.INFO} {...restProps} /> ); -}; +}); + +AssetMetaTag.displayName = 'AssetMetaTag';packages/ui/src/components/asset-info/asset-info.tsx (4)
19-19: Remove unused importAssetName.The
AssetNamecomponent is imported but never used in this file.-import { AssetName } from '../asset-name/asset-name';🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
94-130: Consider reducing duplication in AssetMetaTag usage.The three AssetMetaTag instances share similar props and structure. Consider extracting this into a reusable helper function.
const renderAssetMetaTag = ( type: 'entry' | 'initial' | 'chunk', status: boolean, className?: string ) => { const labels = { entry: 'Entrypoint', initial: 'Initial', chunk: 'Chunk' }; return ( <AssetMetaTag as={CustomComponentLink} {...getBundleAssetsByEntryType(type)} onClick={onClick} tag={type} size="medium" status={status} className={cx(css.assetNameTag, className)} > {labels[type]} </AssetMetaTag> ); };
136-149: Remove unused parameter in renderRunName.The
runNameClassNameparameter is destructured but never used. The className passed to FileName comes from the outer scope.- const { className: runNameClassName, run } = renderRunNameOptions; + const { run } = renderRunNameOptions;🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 137-137: Unused variable, import, function or class
Unused variable runNameClassName.
142-144: Consider consistent title usage in AssetMetaTag components.The small AssetMetaTag components use title prop while the medium ones don't. Consider using consistent props across all instances.
-{run.isEntry && <AssetMetaTag tag="entry" title="Entry" size="small" />} -{run.isInitial && <AssetMetaTag tag="initial" title="Initial" size="small" />} -{run.isChunk && <AssetMetaTag tag="chunk" title="Chunk" size="small" />} +{run.isEntry && <AssetMetaTag tag="entry" title="Entrypoint" size="small" />} +{run.isInitial && <AssetMetaTag tag="initial" title="Initial" size="small" />} +{run.isChunk && <AssetMetaTag tag="chunk" title="Chunk" size="small" />}packages/ui/src/components/entry-info/entry-info.tsx (1)
75-84: Add TypeScript type annotation and documentation.The type and function implementation look good, but could benefit from better type safety and documentation:
+/** + * Props for customizing the rendering of run names + * @property className - Optional CSS class name + * @property run - The run data to render + * @property runSelector - Optional selector key to extract the run name + */ export type RenderRunNameProps = { className?: string; run: ReportMetricRun; runSelector?: string; }; -function defaultRenderRunName(props) { +function defaultRenderRunName(props: RenderRunNameProps): JSX.Element { const { className = '', run, runSelector } = props; return <FileName as="code" name={run[runSelector]} className={className} />; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/ui/src/components/asset-info/asset-info.module.css(1 hunks)packages/ui/src/components/asset-info/asset-info.tsx(4 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css(1 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx(1 hunks)packages/ui/src/components/asset-meta-tag/index.ts(1 hunks)packages/ui/src/components/asset-name/asset-name.module.css(1 hunks)packages/ui/src/components/asset-name/asset-name.tsx(1 hunks)packages/ui/src/components/entry-info/entry-info.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/components/asset-meta-tag/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css
- packages/ui/src/components/asset-name/asset-name.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/ui/src/components/asset-info/asset-info.tsx
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
[notice] 137-137: Unused variable, import, function or class
Unused variable runNameClassName.
🪛 Biome
packages/ui/src/components/asset-name/asset-name.module.css
[error] 1-1: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 10-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (10)
packages/ui/src/components/asset-info/asset-info.module.css (1)
38-40: Consider removing unnecessary empty state handling.
The .runNameTags:empty rule changing display to block seems unnecessary as an empty flex container should collapse naturally. Unless there's a specific edge case being handled, this rule could be removed to simplify the styles.
Let's check if there are any specific cases where this empty state is needed:
packages/ui/src/components/asset-name/asset-name.module.css (3)
3-8: LGTM! Well-structured styles using CSS variables.
The styles for not predictive elements are well-organized and follow good practices:
- Consistent use of CSS variables for spacing and colors
- Semantic color naming with
--color-warning-dark - Clear visual structure with fixed dimensions and spacing
Also applies to: 12-14
16-27: LGTM! Excellent handling of meta tags layout.
The meta tags styling demonstrates several good practices:
- Efficient use of adjacent sibling selector for consistent spacing
- Smart empty state handling to prevent layout issues
- Precise spacing calculations using
calc()
29-32: LGTM! Clean and effective text styling.
The name text styling is appropriately implemented with proper vertical alignment for consistent rendering.
packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (2)
1-7: LGTM! Clean and well-organized imports.
The imports follow a logical order and include all necessary dependencies.
17-23: Enhance status handling in className composition.
The className logic only handles 'added' and 'removed' status values. Consider handling all possible status values or documenting why only these two are handled.
packages/ui/src/components/asset-info/asset-info.tsx (1)
94-104: LGTM! AssetMetaTag implementation looks good.
The replacement of Tag with AssetMetaTag improves the specificity and consistency of asset metadata display.
Also applies to: 107-117, 120-130
packages/ui/src/components/entry-info/entry-info.tsx (3)
5-5: LGTM: Import addition is consistent with new type usage.
The addition of ReportMetricRun to the imports aligns with its usage in the new RenderRunNameProps type.
96-96: LGTM: Well-typed render prop addition.
The new renderRunName prop follows React best practices by:
- Using the render prop pattern for flexibility
- Maintaining backward compatibility with optional typing
- Having proper TypeScript type annotations
161-166:
Fix incorrect text prop in CopyToClipboard component.
The CopyToClipboard component is using item.label instead of the actual run name that's being displayed. This creates inconsistency between what's shown and what's copied.
<FlexStack space="xxxsmall" alignItems="center" className={css.label}>
{renderRunName({
className: css.fileName,
run: rowRun,
runSelector: runNameSelector,
})}
- <CopyToClipboard text={item.label} size="small" />
+ <CopyToClipboard text={rowRun[runNameSelector]} size="small" />
</FlexStack>Likely invalid or redundant comment.
vio
force-pushed
the
asset-meta-tag-changes
branch
from
93d21e8
to
cee02be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/ui/src/components/asset-info/asset-info.tsx (1)
136-149: Consider dependencies array for useCallback.The
renderRunNamefunction is properly memoized with useCallback, but it doesn't specify any dependencies. While this works, explicitly listing dependencies would better communicate the function's dependencies to other developers.- }, []); + }, []); // Explicitly indicate that there are no dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/ui/src/components/asset-info/asset-info.module.css(1 hunks)packages/ui/src/components/asset-info/asset-info.tsx(4 hunks)packages/ui/src/components/entry-info/entry-info.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/asset-info/asset-info.module.css
- packages/ui/src/components/entry-info/entry-info.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/ui/src/components/asset-info/asset-info.tsx
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
🔇 Additional comments (3)
packages/ui/src/components/asset-info/asset-info.tsx (3)
19-19: Remove unused import.
The AssetName import is not used in this file and should be removed.
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
94-104: Well-structured tag rendering implementation.
The replacement of generic tags with AssetMetaTag components improves type safety and consistency. The implementation follows a clear pattern across entry, initial, and chunk tags with proper prop typing.
Also applies to: 107-117, 120-130
Line range hint 154-161: Clean integration of new renderRunName prop.
The addition of the renderRunName prop to EntryInfo is well-integrated and maintains backward compatibility while adding new functionality.
vio
force-pushed
the
asset-meta-tag-changes
branch
from
cee02be
to
f457a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (3)
9-12: Consider refining the status prop type.The
statusprop type allows boolean values, which might be too permissive and could lead to inconsistent usage. Consider restricting it to justReportMetricAssetRowMetaStatusfor better type safety and clearer intent.export type AssetMetaTagProps = { tag: 'entry' | 'initial' | 'chunk'; - status?: ReportMetricAssetRowMetaStatus | boolean; + status?: ReportMetricAssetRowMetaStatus; } & TagProps;
14-34: Consider adding constants and memoization.The component implementation is clean and follows React best practices. Consider these improvements:
- Define constants for tag types to avoid string literals
- Memoize the component to prevent unnecessary re-renders
+const TAG_TYPES = { + ENTRY: 'entry', + INITIAL: 'initial', + CHUNK: 'chunk', +} as const; + -export const AssetMetaTag = (props: AssetMetaTagProps) => { +export const AssetMetaTag = React.memo((props: AssetMetaTagProps) => { const { className = '', title, tag, status, ...restProps } = props; const rootClassName = cx( css.root, status === 'added' && css.added, status === 'removed' && css.removed, css[tag], className, ); return ( <Tag className={rootClassName} title={title} size={Tag.SIZES.SMALL} kind={Tag.KINDS.INFO} {...restProps} /> ); -}; +}); + +AssetMetaTag.displayName = 'AssetMetaTag';
14-16: Add runtime validation for props.While TypeScript provides compile-time type safety, consider adding runtime validation for better error handling in non-TypeScript environments or when receiving props from external sources.
export const AssetMetaTag = (props: AssetMetaTagProps) => { const { className = '', title, tag, status, ...restProps } = props; + + if (!['entry', 'initial', 'chunk'].includes(tag)) { + console.warn(`Invalid tag value: ${tag}`); + } + + if (status && !['added', 'removed'].includes(status.toString())) { + console.warn(`Invalid status value: ${status}`); + }packages/ui/src/components/bundle-assets/bundle-assets.utils.js (1)
45-71: Enhance clarity and conciseness.The function logic is sound, but could be improved:
- The comment "filter empty runs" is misleading as it filters undefined values
- Variable names like
currentandbaselinecould be more descriptive- The logic could be more concise
Consider this refactor:
export const getAssetMetaStatus = (values) => { if (!values.includes(true)) { return false; } - // filter empty runs + // filter undefined values const metaValues = values.filter((value) => typeof value !== 'undefined'); - const current = metaValues[0]; - const metaValuesLength = metaValues.length; + const [firstValue, ...rest] = metaValues; + const lastValue = rest[rest.length - 1]; - if (metaValuesLength === 1) { - return Boolean(current); + if (rest.length === 0) { + return Boolean(firstValue); } - const baseline = metaValues[metaValuesLength - 1]; - - if (current && !baseline) { + if (firstValue && !lastValue) { return 'added'; } - if (!current && baseline) { + if (!firstValue && lastValue) { return 'removed'; } return true; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
packages/ui/src/components/asset-info/asset-info.module.css(1 hunks)packages/ui/src/components/asset-info/asset-info.tsx(5 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css(1 hunks)packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx(1 hunks)packages/ui/src/components/asset-meta-tag/index.ts(1 hunks)packages/ui/src/components/asset-name/asset-name.module.css(1 hunks)packages/ui/src/components/asset-name/asset-name.tsx(1 hunks)packages/ui/src/components/asset-not-predictive/asset-not-predictive.tsx(1 hunks)packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts(1 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(2 hunks)packages/ui/src/components/entry-info/entry-info.tsx(5 hunks)packages/utils/src/webpack/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/components/asset-meta-tag/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/ui/src/components/asset-info/asset-info.module.css
- packages/ui/src/components/asset-meta-tag/asset-meta-tag.module.css
- packages/ui/src/components/asset-name/asset-name.tsx
- packages/ui/src/components/asset-not-predictive/asset-not-predictive.tsx
- packages/ui/src/components/bundle-assets/tests/add-metric-report-asset-row-data.ts
- packages/ui/src/components/entry-info/entry-info.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/ui/src/components/asset-info/asset-info.tsx
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
[notice] 20-20: Unused variable, import, function or class
Unused import FileName.
🪛 Biome
packages/ui/src/components/asset-name/asset-name.module.css
[error] 1-1: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 10-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (13)
packages/ui/src/components/asset-name/asset-name.module.css (3)
3-9: LGTM! Well-structured styles using CSS variables.
The styles follow best practices by:
- Using semantic color variables
- Consistent spacing variables
- Clear and meaningful class names
Also applies to: 12-14
16-27: LGTM! Excellent implementation of meta tags styling.
The implementation demonstrates several good practices:
- Efficient spacing using adjacent selectors
- Proper empty state handling
- Consistent use of spacing variables
29-32: LGTM! Clean and focused text styling.
The nameText class provides appropriate display and alignment properties for consistent text rendering.
packages/ui/src/components/asset-meta-tag/asset-meta-tag.tsx (1)
1-7: LGTM! Well-structured imports.
The imports are well-organized, following the convention of external dependencies first, followed by internal imports. The explicit type imports enhance type safety.
packages/utils/src/webpack/types.ts (3)
32-39: Well-structured interface with improved naming!
The rename from Asset to AssetMetricRun better reflects its purpose and relationship with MetricRun. The interface clearly defines the essential properties of a webpack asset with appropriate types.
40-40: LGTM: Type definition correctly updated!
The Assets type definition has been properly updated to use the renamed AssetMetricRun interface, maintaining type safety and consistency.
32-40: Verify all usages are updated to use the new type names.
Let's ensure all references to the old Asset interface have been updated to use AssetMetricRun.
✅ Verification successful
Let me gather more context about the specific usage patterns to ensure all references are properly updated.
Based on the gathered information, I can now generate the final response:
No issues found with type renaming and usage
The codebase inspection shows that:
- The interfaces found (
MetricsAssets,JobInsightAssetsSizeTotalData,WebpackStatsFilteredAsset,ReportAssets,AssetInfoProps) are distinct types serving different purposes and not related to the oldAssetinterface - All usages of
AssetMetricRunare properly referenced in the codebase - The
Assetstype is correctly defined asRecord<string, AssetMetricRun>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old Asset interface
# and verify all usages of AssetMetricRun
# Search for any remaining references to the old Asset interface
echo "Checking for any remaining references to 'Asset'..."
rg --type ts "interface.*Asset[^M]"
# Search for usages of the new AssetMetricRun interface
echo "Verifying usages of AssetMetricRun..."
rg --type ts "AssetMetricRun"
Length of output: 1316
Script:
#!/bin/bash
# Check the content of files that might still use the old Asset interface
# and verify the context of AssetMetricRun usage
# Check MetricsAssets interface
echo "Checking MetricsAssets interface..."
rg -A 5 "interface MetricsAssets" packages/utils/src/webpack/types.ts
# Check JobInsightAssetsSizeTotalData interface
echo "Checking JobInsightAssetsSizeTotalData interface..."
rg -A 5 "interface JobInsightAssetsSizeTotalData" packages/utils/src/constants.ts
# Check WebpackStatsFilteredAsset interface
echo "Checking WebpackStatsFilteredAsset interface..."
rg -A 5 "interface WebpackStatsFilteredAsset" packages/plugin-webpack-filter/src/index.ts
# Check ReportAssets interface
echo "Checking ReportAssets interface..."
rg -A 5 "interface ReportAssets" packages/cli-utils/src/reports.ts
# Check AssetInfoProps interface
echo "Checking AssetInfoProps interface..."
rg -A 5 "interface AssetInfoProps" packages/ui/src/components/asset-info/asset-info.tsx
Length of output: 1540
packages/ui/src/components/bundle-assets/bundle-assets.utils.js (3)
1-7: LGTM! Well-documented type definitions.
The type definitions and imports are well-structured and provide good TypeScript integration.
79-117: Consider optimizing data processing and adding validation.
The function handles the logic well but could benefit from some improvements:
- Multiple arrays (runsEntry, runsInitial, runsChunk) could be combined into a single data structure
- Missing validation for required properties
- Complex changed status logic could be extracted for better maintainability
Consider this refactor:
export const addMetricReportAssetRowData = (row) => {
const { changed, runs } = row;
+ if (!Array.isArray(runs)) {
+ throw new Error('runs must be an array');
+ }
+
// Collect meta for each run
- const runsEntry = [];
- const runsInitial = [];
- const runsChunk = [];
+ const metadata = runs.map((run) => ({
+ isEntry: run?.isEntry,
+ isInitial: run?.isInitial,
+ isChunk: run?.isChunk,
+ }));
- runs.forEach((run) => {
- runsEntry.push(run?.isEntry);
- runsInitial.push(run?.isInitial);
- runsChunk.push(run?.isChunk);
- });
+ const isEntry = getAssetMetaStatus(metadata.map(m => m.isEntry));
+ const isInitial = getAssetMetaStatus(metadata.map(m => m.isInitial));
+ const isChunk = getAssetMetaStatus(metadata.map(m => m.isChunk));Let's verify the usage context:
#!/bin/bash
# Search for calls to addMetricReportAssetRowData to understand expected inputs
rg "addMetricReportAssetRowData" -B 2 -A 2Line range hint 15-39: Review the predictive check logic.
The function has several potential issues:
- No validation for empty or invalid runs array
- The condition
key !== current.nameseems counterintuitive - why should the key be different from the name? - The chronological order of runs is assumed but not validated
Let's verify the usage context:
Consider adding input validation and clarifying the logic:
export const getIsNotPredictive = (row) => {
const { key, runs } = row;
+
+ if (!Array.isArray(runs) || runs.length < 2) {
+ return false;
+ }
return runs.reduce((agg, current, index) => {packages/ui/src/components/asset-info/asset-info.tsx (3)
12-12: LGTM: Type update aligns with codebase changes.
The type change from Asset to AssetMetricRun correctly reflects the type renaming mentioned in the summary.
94-105: LGTM: Consistent AssetMetaTag implementation.
The replacement of Tag with AssetMetaTag is well-implemented with consistent prop usage across entry, initial, and chunk tags. The new component provides better semantics and type safety.
Also applies to: 107-118, 120-131
137-153: Review useCallback dependencies.
The useCallback hook has an empty dependency array, but the function uses external values (AssetMetaTag component). Consider if any dependencies should be added to prevent stale closures.
| .root {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or utilize the empty root class.
Empty CSS blocks increase bundle size without providing value. Consider either:
- Removing the
.rootclass if it's not needed - Adding necessary styles if it serves as a component wrapper
🧰 Tools
🪛 Biome
[error] 1-1: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
| height: var(--space-small); | ||
| } | ||
|
|
||
| .notPredictiveAnchor {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or utilize the empty notPredictiveAnchor class.
Empty CSS blocks increase bundle size without providing value. If this class is needed for JavaScript targeting, consider adding a comment explaining its purpose.
🧰 Tools
🪛 Biome
[error] 10-10: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
| @@ -130,6 +160,7 @@ | |||
| labels={labels} | |||
| tags={tags} | |||
| onClose={onClose} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove 'as any' type assertion.
The type assertion item as any bypasses TypeScript's type checking. Consider properly typing the EntryInfo component's props or fixing the type mismatch.
Would you like assistance in properly typing this component?
| import { AssetName } from '../asset-name/asset-name'; | ||
| import { FileName } from '../../ui'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports.
The following imports are not used in this file:
AssetNamefrom '../asset-name/asset-name'FileNamefrom '../../ui'
-import { AssetName } from '../asset-name/asset-name';
-import { FileName } from '../../ui';📝 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.
| import { AssetName } from '../asset-name/asset-name'; | |
| import { FileName } from '../../ui'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 19-19: Unused variable, import, function or class
Unused import AssetName.
[notice] 20-20: Unused variable, import, function or class
Unused import FileName.
vio
force-pushed
the
asset-meta-tag-changes
branch
from
f457a9f
to
5030e78
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/ui/src/components/bundle-assets/bundle-assets.utils.js (3)
Line range hint
15-39: Consider improving function clarity and efficiency.A few suggestions to enhance this function:
- The JSDoc comment could better explain what "not predictive" means in this context.
- The logic could be simplified using array methods like
some().- The
key !== current.namecheck seems redundant as it's followed bycurrent.name === runs[index + 1].name.Consider this more concise implementation:
export const getIsNotPredictive = (row) => { const { runs } = row; - return runs.reduce((agg, current, index) => { - if (agg) { - return agg; - } - if (index + 1 === runs.length) { - return agg; - } - if ( - current && - runs[index + 1] && - key !== current.name && - current.name === runs[index + 1].name && - current.value !== runs[index + 1].value - ) { - return true; - } - return agg; - }, false); + return runs.some((current, index) => + index < runs.length - 1 && + current?.name === runs[index + 1]?.name && + current?.value !== runs[index + 1]?.value + ); };
45-71: LGTM! Consider minor readability improvements.The function logic is solid and handles edge cases well. A few minor suggestions to improve readability:
export const getAssetMetaStatus = (values) => { if (!values.includes(true)) { return false; } - // filter empty runs const metaValues = values.filter((value) => typeof value !== 'undefined'); - const current = metaValues[0]; + const firstRun = metaValues[0]; const metaValuesLength = metaValues.length; if (metaValuesLength === 1) { - return Boolean(current); + return Boolean(firstRun); } - const baseline = metaValues[metaValuesLength - 1]; + const lastRun = metaValues[metaValuesLength - 1]; - if (current && !baseline) { + if (firstRun && !lastRun) { return 'added'; } - if (!current && baseline) { + if (!firstRun && lastRun) { return 'removed'; } return true; };
79-117: LGTM! Consider enhancing documentation.The function implementation is solid with good separation of concerns. However, the complex logic for
assetChangedcould benefit from better documentation.Add a more detailed JSDoc comment explaining the logic:
/** * Add asset row flags + * + * @description + * Processes a metric row to determine various asset flags and metadata: + * - Collects entry, initial, and chunk flags for each run + * - Determines meta status for each flag type + * - Marks asset as changed if either: + * - Original changed flag is true + * - Any meta tag status has changed (added/removed) * * @param {ReportMetricRow} row * @returns {ReportMetricAssetRow} */packages/ui/src/components/asset-info/asset-info.tsx (1)
56-74: Consider making children prop optional.The AssetRunName component is well-structured and enhances reusability. However, consider making the children prop optional if there are cases where only tags need to be rendered.
type AssetRunNameProps = { run: ReportMetricAssetRow; - children: ReactNode; + children?: ReactNode; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/ui/src/components/asset-info/asset-info.tsx(7 hunks)packages/ui/src/components/bundle-assets/__tests__/add-metric-report-asset-row-data.ts(1 hunks)packages/ui/src/components/bundle-assets/bundle-assets.utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/bundle-assets/tests/add-metric-report-asset-row-data.ts
🔇 Additional comments (5)
packages/ui/src/components/bundle-assets/bundle-assets.utils.js (1)
1-7: LGTM! Well-documented type definitions.
The JSDoc type definitions provide clear documentation and improve IDE support for TypeScript-like type checking.
packages/ui/src/components/asset-info/asset-info.tsx (4)
Line range hint 1-21: LGTM! Import statements are well-organized.
The imports have been properly updated to reflect the new type system and component structure. Previous static analysis warnings about unused imports have been addressed.
Line range hint 22-54: LGTM! Component types are properly defined.
The ChunkModulesLink component has been improved with better type definitions using React's built-in ComponentProps.
113-151: LGTM! Tag rendering is consistent and well-structured.
The tag rendering logic has been improved with the new AssetMetaTag component, providing consistent styling and behavior across different tag types.
163-164:
Remove type assertion and properly type the EntryInfo props.
The as any type assertion should be removed to maintain type safety. Consider properly typing the EntryInfo component's props.
- item={item as any}
+ item={item}To fix this, ensure that the EntryInfo component's props properly match the item type.
Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
AssetMetaTag,AssetName, and enhancedAssetNotPredictive.App,AssetName, andEntryInfocomponents.getAssetMetaStatusfor improved asset metadata handling.Bug Fixes
Documentation
Style
Chores