refactor: optimisations #3523
refactor: optimisations #3523
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: b86e5a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve the introduction of several new utility functions in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![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 (2)
packages/components/modal/src/modal-transition.ts (2)
29-29: Approved with suggestion: Consider consistency inwillChangeusageThe addition of
willChange: "transform"to the exit state is a good optimization, specifically targeting the transform property which is the main focus of the exit animation. This should help with performance during the modal's closing transition.However, for consistency with the enter state and to account for the opacity animation, you might consider using
willChange: "auto"here as well. This would allow the browser to optimize for all changing properties, including both transform and opacity.Consider changing to:
- willChange: "transform", + willChange: "auto",This maintains consistency with the enter state and covers all animated properties.
Line range hint
1-36: Overall: Effective optimization addressing the blurry text issueThese changes to the
scaleInOuttransition object represent a targeted approach to resolving the blurry text issue reported in #3817. By addingwillChangeproperties to both the enter and exit states, you're providing valuable hints to the browser for optimizing the modal's rendering and animation performance.The use of
willChange: "auto"for the enter state andwillChange: "transform"for the exit state should effectively address the Chrome rendering issue with CSS transforms and percentage-based values. This solution is likely to result in sharper text rendering when the modal is opened, improving the overall user experience without requiring additional actions like window resizing.While there's a slight inconsistency in the
willChangevalues between enter and exit states, both approaches are valid and should yield performance improvements. Great job on implementing these optimizations!To further improve this component, consider:
- Monitoring the performance impact of these changes in real-world usage.
- If needed, fine-tune the
willChangeproperties based on performance profiling results.- Ensuring that these optimizations are consistently applied across other similar transition components in the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/modal/src/modal-transition.ts (2 hunks)
🔇 Additional comments (1)
packages/components/modal/src/modal-transition.ts (1)
8-8: Approved: Good performance optimization for modal entryThe addition of
willChange: "auto"to the enter state is a smart optimization. This hints to the browser that it should prepare for changes in any animatable properties during the modal's entry animation. It's likely to help with the blurry text issue mentioned in #3817 by encouraging the browser to create a new compositing layer, which can lead to sharper text rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/utilities/shared-utils/src/functions.ts (4)
23-25: Approve changes with a minor suggestionThe updates to the
capitalizefunction improve its behavior by handling falsy inputs and ensuring consistent capitalization. However, consider using a more descriptive parameter name thans.Consider renaming the parameter for better clarity:
-export const capitalize = (s: string) => { - return s ? s.charAt(0).toUpperCase() + s.slice(1).toLowerCase() : ""; +export const capitalize = (text: string) => { + return text ? text.charAt(0).toUpperCase() + text.slice(1).toLowerCase() : ""; };
212-218: ApproveuniqByfunction with a type safety improvement suggestionThe
uniqByfunction is a valuable addition to the utility library. It efficiently filters unique elements based on a flexible iteratee. However, we can improve type safety by utilizing theIteratee<T>type defined earlier.Consider updating the function signature for better type safety:
-export function uniqBy<T>(arr: T[], iteratee: any) { +export function uniqBy<T>(arr: T[], iteratee: Iteratee<T>) { if (typeof iteratee === "string") { iteratee = (item: T) => item[iteratee as keyof T]; } return arr.filter((x, i, self) => i === self.findIndex((y) => iteratee(x) === iteratee(y))); }
254-256: ApprovekebabCasefunction with suggestions for improvementThe
kebabCasefunction provides a simple and effective way to convert strings to kebab-case. However, there are a few edge cases that the current implementation doesn't handle.Consider the following improvements to handle more cases:
- Handle PascalCase strings.
- Handle multiple consecutive uppercase letters.
- Handle non-alphanumeric characters.
Here's an improved implementation:
export const kebabCase = (s: string) => { return s .replace(/([a-z0-9])([A-Z])/g, '$1-$2') .replace(/([A-Z])([A-Z])(?=[a-z])/g, '$1-$2') .replace(/[^a-zA-Z0-9]/g, '-') .toLowerCase(); };This implementation will correctly handle cases like:
- "fooBar" → "foo-bar"
- "FooBar" → "foo-bar"
- "FOOBar" → "foo-bar"
- "foo_bar" → "foo-bar"
269-276: ApprovemapKeysfunction with type improvement suggestionsThe
mapKeysfunction is a useful addition that efficiently transforms object keys. The implementation is concise and effective.To improve type safety and clarity, consider updating the type annotations:
export const mapKeys = <T extends Record<string, unknown>, U extends string | number | symbol>( obj: T, iteratee: (value: T[keyof T], key: string) => U ): Record<U, T[keyof T]> => { return Object.fromEntries( Object.entries(obj).map(([key, value]) => [iteratee(value, key), value]) ) as Record<U, T[keyof T]>; };This updated signature:
- Ensures that the input object has string keys and any value type.
- Allows the iteratee to return any valid object key type (string, number, or symbol).
- Correctly types the return object based on the iteratee's return type and the original value types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/utilities/shared-utils/src/functions.ts (6 hunks)
🔇 Additional comments (5)
packages/utilities/shared-utils/src/functions.ts (5)
11-12: LGTM: Well-definedIterateetype aliasThe
Iteratee<T>type alias is a good addition. It provides flexibility by allowing either a function or a key of T to be used as an iteratee in various utility functions. This enhances type safety and improves the overall API design.
178-196: LGTM: Well-implementeddebouncefunctionThe
debouncefunction is a great addition to the utility library. It's well-implemented with proper TypeScript generics, maintaining the original function's context and arguments. The implementation follows best practices for debouncing and provides a useful tool for performance optimization in event-driven scenarios.
342-391: LGTM: Well-implementedintersectionByfunctionThe
intersectionByfunction is a robust and efficient implementation for computing the intersection of multiple arrays based on a transformed value. Key points:
- Proper use of the
Iteratee<T>type enhances type safety.- Efficient use of Sets for performance optimization.
- Thorough error handling for invalid inputs.
- Correct handling of both function and string iteratees.
The implementation is well-thought-out and should perform well for various use cases.
Line range hint
1-391: Summary: Excellent enhancements to the utility functionsThis PR significantly improves the
functions.tsfile by adding several new utility functions and enhancing existing ones. The changes align well with the PR objectives of optimizing the codebase:
- The new functions (
debounce,uniqBy,omit,kebabCase,mapKeys,get, andintersectionBy) provide valuable tools for common programming tasks, potentially reducing the need for external libraries.- Existing functions have been improved and better documented, enhancing maintainability.
- Type safety has been improved in most areas, particularly with the introduction of the
Iteratee<T>type.These enhancements should contribute to a more efficient and maintainable codebase. The utility functions are well-implemented and should help reduce bundle size by eliminating the need for some external dependencies.
Minor suggestions for further improvement:
- Consider implementing deep cloning in the
omitfunction if needed for your use cases.- Enhance the
kebabCasefunction to handle more edge cases.- Further improve type annotations in some functions, as suggested in the individual reviews.
Overall, these changes represent a significant improvement to the shared utilities and should positively impact the project's performance and maintainability.
234-242: Approveomitfunction with a note on shallow copyingThe
omitfunction is a well-implemented utility that creates a new object by omitting specified keys. The use of TypeScript generics and theOmitutility type ensures type safety.Note that the current implementation creates a shallow copy of the object. This is sufficient for most use cases, but be aware that nested objects will still share references with the original object. If deep cloning is required for your use case, consider using a deep clone utility or library.
To verify the shallow copy behavior, you can run the following test:
This test will demonstrate the shallow copy behavior of the
omitfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/docs/components/docs/components/codeblock.tsx (1)
Line range hint
125-134: Optimize debounce implementation to prevent memory leaks.The current implementation recreates the debounced function on every render. Consider:
- Moving the debounced function creation outside the effect
- Ensuring pending debounced calls are cancelled in cleanup
+ const debouncedHandleSelectionChange = React.useMemo( + () => debounce(handleSelectionChange, 1000), + [] + ); useEffect(() => { - const debouncedHandleSelectionChange = debounce(handleSelectionChange, 1000); document.addEventListener("selectionchange", debouncedHandleSelectionChange); return () => { + debouncedHandleSelectionChange.cancel?.(); document.removeEventListener("selectionchange", debouncedHandleSelectionChange); }; - }, []); + }, [debouncedHandleSelectionChange]);apps/docs/components/navbar.tsx (1)
Line range hint
210-214: Add error handling for tracking eventsWhile the tracking implementation is thorough, consider adding error handling to prevent potential issues from affecting the user experience.
const handlePressNavbarItem = (name: string, url: string) => { - trackEvent("NavbarItem", { - name, - action: "press", - category: "navbar", - data: url, - }); + try { + trackEvent("NavbarItem", { + name, + action: "press", + category: "navbar", + data: url, + }); + } catch (error) { + console.error("Failed to track navbar interaction:", error); + } };Also applies to: 221-225, 232-236
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/docs/components/docs/components/codeblock.tsx(1 hunks)apps/docs/components/navbar.tsx(4 hunks)apps/docs/components/sandpack/use-sandpack.ts(2 hunks)apps/docs/package.json(1 hunks)packages/components/accordion/package.json(3 hunks)packages/components/autocomplete/package.json(2 hunks)packages/components/button/package.json(1 hunks)packages/components/calendar/package.json(2 hunks)packages/components/card/package.json(1 hunks)packages/components/dropdown/package.json(2 hunks)packages/components/modal/package.json(3 hunks)packages/components/navbar/package.json(2 hunks)packages/components/popover/package.json(3 hunks)packages/components/select/package.json(2 hunks)packages/components/snippet/package.json(1 hunks)packages/components/tabs/package.json(2 hunks)packages/components/tabs/src/tab.tsx(1 hunks)packages/components/tooltip/package.json(3 hunks)packages/core/react/package.json(1 hunks)packages/core/system/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/tabs/src/tab.tsx
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/docs/components/sandpack/use-sandpack.ts
- apps/docs/package.json
- packages/components/accordion/package.json
- packages/components/autocomplete/package.json
- packages/components/button/package.json
- packages/components/calendar/package.json
- packages/components/card/package.json
- packages/components/dropdown/package.json
- packages/components/modal/package.json
- packages/components/navbar/package.json
- packages/components/popover/package.json
- packages/components/select/package.json
- packages/components/snippet/package.json
- packages/components/tabs/package.json
- packages/components/tooltip/package.json
- packages/core/react/package.json
- packages/core/system/package.json
🔇 Additional comments (4)
apps/docs/components/docs/components/codeblock.tsx (2)
Line range hint 164-165: LGTM: Clean props handling with omit.
The implementation correctly removes the 'key' prop before spreading while maintaining unique keys through getUniqueID. This is a good practice to avoid React key warnings while ensuring proper component rendering.
Also applies to: 189-190
4-4: Verify the behavior of the new utility functions.
The change from lodash to @nextui-org/shared-utils aligns with the PR's optimization goals. However, we should verify that the new implementations maintain the same behavior, especially for:
- The debounce timing in the selection change handler
- The omit function's handling of props spreading
✅ Verification successful
The utility functions implementation is safe to use
The implementation of both debounce and omit in @nextui-org/shared-utils follows standard patterns and maintains expected behavior:
- The
debouncefunction properly handles timeouts and function context, matching the behavior needed for the selection change handler (1000ms delay) - The
omitfunction correctly removes specified keys from objects, which is suitable for the props spreading use cases in the component
The usage in the codeblock component aligns well with these implementations:
debounceis used with the correct timing for selection changesomitis used appropriately for filtering out specific props before spreading
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of debounce and omit in shared-utils
# Test 1: Check the debounce implementation
echo "Checking debounce implementation..."
ast-grep --pattern 'export function debounce'
# Test 2: Check the omit implementation
echo "Checking omit implementation..."
ast-grep --pattern 'export function omit'
# Test 3: Check for any other components using these utilities
echo "Checking other components using these utilities..."
rg "import.*\{.*(debounce|omit).*\}.*from.*@nextui-org/shared-utils"
Length of output: 842
Script:
#!/bin/bash
# Let's check the actual implementations to verify behavior
# Check the shared-utils implementation
echo "Searching for shared-utils implementations..."
fd "shared-utils" --type f --exec rg -A 10 "export (const|function) (debounce|omit)"
# Check usage in codeblock component
echo "Checking usage in codeblock component..."
rg -A 5 "(debounce|omit)" apps/docs/components/docs/components/codeblock.tsx
Length of output: 1478
Script:
#!/bin/bash
# Let's find the actual implementation in shared-utils package
echo "Looking for shared-utils package implementation..."
fd -t f -e ts -e js . --full-path "**/shared-utils/**" --exec cat {}
# Also check if there are any tests for these utilities
echo "Looking for related tests..."
fd -t f -e test.ts -e test.js -e spec.ts -e spec.js . --full-path "**/shared-utils/**" --exec cat {}
Length of output: 567
Script:
#!/bin/bash
# Let's try a different approach to find the shared-utils package
echo "Finding shared-utils package location..."
fd shared-utils
# Once we find it, let's look for the specific files
echo "Looking for utility implementations..."
rg -A 10 "^export (const|function) (debounce|omit)" --type ts
Length of output: 3337
apps/docs/components/navbar.tsx (2)
199-199: LGTM: Effective optimization of includes checks
The replacement of lodash's includes with native JavaScript includes is a good optimization that reduces bundle size while maintaining the same functionality.
Also applies to: 210-210, 221-221, 232-232
Line range hint 313-332: Verify text rendering in version dropdown animation
Given the reported issues with blurry text during animations (#3817, #2288), we should verify that the AnimatePresence transition doesn't cause similar text rendering issues.
✅ Verification successful
Let me gather more specific information about the animation implementation in the navbar component.
Based on the search results, I can now provide a final response:
No text rendering issues expected with current animation implementation
The AnimatePresence animation in the navbar only uses opacity transitions (0 to 1) without any transform properties or percentage-based values that could cause text blur. The implementation is using simple fade animations that don't affect text rendering quality.
- The motion.div at L196-202 only includes
opacityin its animation properties:animate={{opacity: 1}} exit={{opacity: 0}} initial={{opacity: 0}} - No transform properties or complex animations are used in the version display component
- The CSS files only show transform usage in unrelated components (nprogress.css)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any will-change or transform properties that might affect text rendering
rg -g '*.{css,scss}' 'will-change|transform:'
# Look for any framer-motion animation configurations that might use percentage-based transforms
ast-grep --pattern 'animate = {
$$$
opacity: $_
$$$
}'
Length of output: 330
Script:
#!/bin/bash
# Search for AnimatePresence usage in navbar and related components
rg -A 5 "AnimatePresence" apps/docs/components/navbar.tsx
# Look for motion component configurations that might affect text rendering
ast-grep --pattern 'motion.$_ = {
$$$
transition: $_
$$$
}'
# Check for any transform-related styles in the navbar
rg -g '*.{css,scss,tsx}' 'transform' apps/docs/components/navbar
Length of output: 1102
* refactor: replace lodash with native approaches
* refactor(deps): update framer-motion versions
* feat(utilities): add @nextui-org/dom-animation
* refactor(components): load domAnimation dynamically
* refactor(deps): add @nextui-org/dom-animation
* fix(utilities): relocate index.ts
* feat(changeset): framer motion optimization
* chore(deps): bump framer-motion version
* fix(docs): conflict issue
* refactor(hooks): remove the unnecessary this aliasing
* refactor(utilities): remove the unnecessary this aliasing
* chore(docs): remove {} so that it won't be true all the time
* chore(dom-animation): end with new line
* refactor(hooks): use debounce from `@nextui-org/shared-utils`
* chore(deps): add `@nextui-org/shared-utils`
* refactor: move mapKeys logic to `@nextui-org/shared-utils`
* refactor: use `get` from `@nextui-org/shared-utils`
* refactor(docs): use `get` from `@nextui-org/shared-utils`
* refactor(shared-utils): mapKeys
* chore(deps): bump framer-motion version
* chore(deps): remove lodash
* refactor(docs): use intersectionBy from shared-utils
* feat(shared-utils): add intersectionBy
* chore(dom-animation): remove extra blank line
* refactor(shared-utils): revise intersectionBy
* fix(modal): add willChange
* refactor(shared-utils): add comments
* fix: build & tests
---------
Co-authored-by: Junior Garcia <[email protected]>
📝 Description
(1) replace lodash with native approaches
(2) framer motion
usedomAnimationinstead ofdomMaxin Tabs componentAffected components:
Unchanged components:
For tabs component, we need to use
domMaxto achieve the shifting when switching between tabs. We cannot use dynamic load here as it fails sometimes, e.g. the animation will be gone sometimes. probably a bug in framer-motion.⛳️ Current behavior (updates)
vite-template + 8 above-mentioned components
🚀 New behavior
vite-template + 8 above-mentioned components
/docincludes the above-mentioned 8 components that use LazyMotion.💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
debounce,uniqBy,omit,kebabCase,mapKeys,get, andintersectionByto enhance functionality.@nextui-org/dom-animationfor improved DOM animations.Bug Fixes
testGoodContrastfunction for better clarity and maintainability in color testing.Chores
framer-motionto version11.9.0.