fix: algolia search icon on homepage #3373
Conversation
amanbhoria
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes in this pull request focus on the Changes
Suggested labels
Suggested reviewers
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 (
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3373--asyncapi-website.netlify.app/ |
amanbhoria
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3373 +/- ##
=======================================
Coverage 67.77% 67.77%
=======================================
Files 21 21
Lines 664 664
=======================================
Hits 450 450
Misses 214 214 ☔ View full report in Codecov by Sentry. |
![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 (1)
components/navigation/NavBar.tsx (1)
Line range hint
82-85: Consider applying the same fix to mobile search button.For consistency, consider using the same function-based approach for the IconLoupe in the mobile search button implementation.
<SearchButton className='flex items-center space-x-2 rounded-md p-2 text-left text-gray-400 transition duration-150 ease-in-out hover:bg-gray-100 hover:text-gray-500 focus:bg-gray-100 focus:text-gray-500 focus:outline-none' aria-label='Open Search' > - <IconLoupe /> + {() => <IconLoupe />} </SearchButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/navigation/NavBar.tsx(1 hunks)
🔇 Additional comments (2)
components/navigation/NavBar.tsx (2)
226-226: LGTM! Fix for Algolia search icon rendering.
The change correctly implements the children prop as a function, which is the expected pattern for the Algolia SearchButton component.
Line range hint 82-85: Verify SearchButton children prop requirements.
Let's verify the SearchButton component's implementation to ensure consistent usage of the children prop.
Also applies to: 226-228
✅ Verification successful
SearchButton component's children prop is correctly implemented
Based on the codebase analysis, the SearchButton component in components/AlgoliaSearch.tsx accepts children as either React.ReactNode or a render prop function with actionKey parameter. The usage in NavBar.tsx is consistent with this implementation:
- Line 226-228 uses IconLoupe as children directly
- Other usages in the codebase demonstrate both patterns:
- Direct children:
<IconLoupe /> - Render prop:
{({ actionKey }) => <IconLoupe />}
- Direct children:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SearchButton component definition and usage
echo "Searching for SearchButton component definition..."
rg -A 10 "export.*SearchButton" --type tsx
echo "Searching for other SearchButton usages..."
rg -A 3 "<SearchButton" --type tsx
Length of output: 343
Script:
#!/bin/bash
# Search for SearchButton component definition and usage with correct file extensions
echo "Searching for SearchButton component definition..."
rg -A 10 "export.*SearchButton" --type ts --type tsx --type jsx --type js
echo "Searching for SearchButton usages..."
rg -A 3 "<SearchButton" --type ts --type tsx --type jsx --type js
# Also try finding the file by name
echo "Finding SearchButton related files..."
fd -e ts -e tsx -e js -e jsx "SearchButton"
Length of output: 522
Script:
#!/bin/bash
# Try simpler search patterns without file type restrictions
echo "Searching for SearchButton definition..."
rg -l "SearchButton"
echo "Searching for SearchButton component content..."
rg -A 10 "SearchButton"
# Try finding any file containing 'search' case-insensitive
echo "Finding search-related files..."
fd -i search
Length of output: 16028
components/navigation/NavBar.tsx
Outdated
| @@ -223,7 +223,7 @@ export default function NavBar({ className = '', hideLogo = false }: NavBarProps | |||
| className='mr-2 flex items-center space-x-2 rounded-md p-2 text-left text-gray-400 transition duration-150 ease-in-out hover:bg-gray-100 hover:text-gray-500 focus:bg-gray-100 focus:text-gray-500 focus:outline-none' | |||
| aria-label='Open Search' | |||
| > | |||
| <IconLoupe /> | |||
| {() => <IconLoupe />} | |||
There was a problem hiding this comment.
@amanbhoria can you please elaborate this change a little
There was a problem hiding this comment.
@sambhavgupta0705 As you can see in the useEffect, type of children is expected as a function, but it was a node earlier. That's the reason why Algolia search icon was not visible.
There was a problem hiding this comment.
@amanbhoria Can we change the useEffect function then, to fix this rendering?
There was a problem hiding this comment.
@akshatnema As it can handle both the searches on landing page simultaneously. Can we do something like that?
There was a problem hiding this comment.
Yeah, can you please try that and see if we are facing any errors or not.
There was a problem hiding this comment.
As far as the search's concern, everything seems to be working.
| if (typeof children === 'function') { | ||
| setChildren(children({ actionKey })); | ||
| } | ||
| setChildren(typeof children === 'function' ? children({ actionKey }) : children); |
There was a problem hiding this comment.
Add missing dependencies to useEffect.
The effect uses children and actionKey but they're not included in the dependency array. This violates the React hooks exhaustive deps rule and could lead to stale closures.
Apply this fix:
setChildren(typeof children === 'function' ? children({ actionKey }) : children);
- }, []);
+ }, [children, actionKey]);Note: If you need the effect to run only once, consider memoizing the children and actionKey values using useMemo.
📝 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.
| setChildren(typeof children === 'function' ? children({ actionKey }) : children); | |
| setChildren(typeof children === 'function' ? children({ actionKey }) : children); | |
| }, [children, actionKey]); |
There was a problem hiding this comment.
the component is being rendered on every button click on Search and so as useEffect. so what's the need to add these parameters in the dependency array? It doesn't make any sense to me
There was a problem hiding this comment.
@amanbhoria, understood. If the component re-renders on every button click and the useEffect runs accordingly, then adding these dependencies might not be necessary in this case. Thank you for the clarification.
✏️ Learnings added
Learnt from: amanbhoria
PR: asyncapi/website#3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.237Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@amanbhoria The solution is working but I am seeing a lot of re-renders in the Chrome DevTools. These were quite low on the master branch deployment |
|
@anshgoyalevil Did you see these logs when running the project locally? I’m only seeing two render logs in the dev tools. Could you help me with this? |
Fix for the issue - #3371
Before: Algolia search icon went missing.
After:
Fix: sending the expected value in children prop
Summary by CodeRabbit
Refactor
childrenprop in theSearchButtoncomponent for improved readability.Bug Fixes
AlgoliaSearchcomponent, ensuring consistent search operations.