fix: Searchbar results. Added pre push tests. #1329
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (4)
.husky/pre-push (2)
1-7: Add error handling and exit code checks.While the basic structure is good, the script needs to be more robust to prevent pushes when tests or type checking fail.
Here's a more resilient implementation:
#!/bin/bash +set -eo pipefail + +# Check if pnpm is available +if ! command -v pnpm &> /dev/null; then + echo "Error: pnpm is not installed" + exit 1 +fi + +# Verify frontend workspace exists +if ! pnpm --filter frontend ls &> /dev/null; then + echo "Error: frontend workspace not found" + exit 1 +fi + echo "### Running pre-push hook ### " + echo "Running Playwright tests..." -pnpm --filter frontend test:playwright +if ! pnpm --filter frontend test:playwright; then + echo "Error: Playwright tests failed" + exit 1 +fi + echo "Running typechecking..." -pnpm --filter frontend typecheck +if ! pnpm --filter frontend typecheck; then + echo "Error: Type checking failed" + exit 1 +fi + echo "Done. Will now push to origin." +exit 0Changes include:
- Added error handling for command availability and workspace existence
- Added proper exit code checking for each command
- Added
set -eo pipefailfor stricter error handling- Added explicit successful exit
4-4: Consider adding timeout limits for long-running operations.The Playwright tests and type checking operations could potentially hang. Consider adding timeout limits to prevent indefinite waiting.
Example implementation:
# Function to run command with timeout run_with_timeout() { timeout 5m "$@" return $? } # Usage run_with_timeout pnpm --filter frontend test:playwrightAlso applies to: 6-6
packages/frontend/src/components/Header/SearchDropdown.tsx (1)
Line range hint
55-71: Consider performance and maintainability improvements.A few suggestions to enhance the code:
- Consider memoizing the search results rendering to prevent unnecessary re-renders
- Extract the magic number
5inslice(0, 5)to a named constantHere's how you could implement these improvements:
+const MAX_SEARCH_RESULTS = 5; +const MemoizedSearchResults = React.memo(({ results, onClose, formatDistance }) => ( + results.slice(0, MAX_SEARCH_RESULTS).map((item) => ( + <SearchDropdownItem key={item.id}> + <InboxItem + key={item.id} + checkboxValue={item.id} + title={item.title} + summary={item.summary} + sender={item.sender} + receiver={item.receiver} + metaFields={item.metaFields} + linkTo={item.linkTo} + onClose={onClose} + isUnread={!item.isSeenByEndUser} + isMinimalistic + /> + <div className={cx(styles.rightContent)}> + <span className={styles.timeSince}> + {autoFormatRelativeTime(new Date(item.updatedAt), formatDistance)} + </span> + <Avatar + name={item.sender.name} + profile={item.sender.isCompany ? 'organization' : 'person'} + imageUrl={item.sender.imageURL} + size="small" + /> + </div> + </SearchDropdownItem> + )) +)); {/* Search results: */} {isFetching ? ( <SearchDropdownSkeleton numberOfItems={3} /> ) : ( - searchResults?.slice(0, 5).map((item) => ( - <SearchDropdownItem key={item.id}> - ... - </SearchDropdownItem> - )) + <MemoizedSearchResults + results={searchResults ?? []} + onClose={handleClose} + formatDistance={formatDistance} + /> )}packages/frontend/src/api/useDialogs.tsx (1)
117-117: Consider removing the lint ignore comment.The added dependencies
partiesandorganizationsare correctly included as they are used within the effect's callback. Since all dependencies are now properly declared, the lint ignore commentbiome-ignore lint/correctness/useExhaustiveDependenciesappears unnecessary.Apply this diff to remove the unnecessary comment:
- // biome-ignore lint/correctness/useExhaustiveDependencies: Full control of what triggers this code is needed useEffect(() => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.husky/pre-push(1 hunks)packages/frontend/src/api/useDialogs.tsx(1 hunks)packages/frontend/src/components/Header/SearchDropdown.tsx(1 hunks)
🔇 Additional comments (4)
.husky/pre-push (1)
1-7: Verify the hook's execution permissions.
Ensure the script has the correct execution permissions set.
✅ Verification successful
Pre-push hook has correct execution permissions
The file .husky/pre-push has the required executable permissions set (rwx), allowing it to run properly during git push operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the pre-push hook has correct permissions
# Test: Check if the file is executable
ls -l .husky/pre-push | grep -q "^-rwx" && echo "Permissions OK" || echo "Missing execute permission"
Length of output: 80
packages/frontend/src/components/Header/SearchDropdown.tsx (1)
27-31: LGTM! The changes align with the PR objectives.
The modification to use selectedParties instead of parties correctly implements the requirement to filter search results based on the selected party.
Let's verify the implementation of useSearchDialogs to ensure it properly handles the parties parameter:
✅ Verification successful
The useSearchDialogs hook correctly utilizes the parties parameter
The implementation confirms that the parties parameter is properly used in the hook:
- It maps the party URIs from the provided parties array (
partyURIs = parties.map((party) => party.party)) - Uses these URIs in the query key and query function for fetching dialogs (
queryKey,searchDialogs(partyURIs, ...)) - Applies the parties data when mapping dialog DTOs to inbox items (
mapDialogDtoToInboxItem(..., parties, ...))
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useSearchDialogs implementation
# Expected: The hook should use the parties parameter in its query/filter logic
# Search for the useSearchDialogs implementation
ast-grep --pattern 'export const useSearchDialogs = $_'
Length of output: 2349
packages/frontend/src/api/useDialogs.tsx (2)
117-117: Changes align well with PR objectives.
The addition of parties and organizations to the dependency array ensures that search results are properly filtered based on the selected party, which directly addresses the PR's goal of enhancing searchbar functionality.
117-117: Verify potential performance impact.
The addition of parties and organizations to the dependency array is correct and aligns with the PR's objective to filter search results by selected party. However, frequent updates to these dependencies could trigger unnecessary re-renders.
Let's analyze the update frequency of these dependencies:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the performance concerns:
Performance impact is minimal - dependencies have low update frequency
The codebase analysis shows that both parties and organizations are stable dependencies:
partiesis primarily set through user selection viasetSelectedPartiesorganizationsis fetched once and cached (with 24h Redis TTL on backend)- Both are used for filtering search results, which is the intended behavior
The re-renders triggered by these dependencies are necessary for correct functionality and don't pose a performance concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how often parties and organizations are updated
# Look for state updates or API calls that modify these values
# Search for parties/organizations state updates
echo "Searching for parties state updates..."
rg -A 3 "setParties|parties\s*=|parties:" --type typescript
echo "Searching for organizations state updates..."
rg -A 3 "setOrganizations|organizations\s*=|organizations:" --type typescript
# Look for related API calls
echo "Searching for API endpoints that might update these values..."
rg -A 3 "useQuery.*parties|useQuery.*organizations" --type typescript
Length of output: 637
Script:
#!/bin/bash
# Let's try without typescript file type specification
echo "Searching for parties state updates..."
rg -A 3 "setParties|parties\s*=|parties:"
echo "Searching for organizations state updates..."
rg -A 3 "setOrganizations|organizations\s*=|organizations:"
echo "Searching for API endpoints that might update these values..."
rg -A 3 "useQuery.*parties|useQuery.*organizations"
# Also search for the context where these dependencies are used
echo "Searching for the component using these dependencies..."
rg -A 10 -B 10 "\[setSearchResults, data\?\?\.searchDialogs\?\?.items, enabled, parties, organizations\]"
Length of output: 15627
I was not aware of an already existing pre-commit hook that does this, only (optional) pre-commit hook I can find runs The tests in this PR are already only running on frontend, running tests in the /packages/frontend/tests folder: pnpm --filter frontend test:playwright echo "Running typechecking..." pnpm --filter frontend typecheck` But I guess everything has to be 100% how you want it or it will never get merged to main, so I will remove all hooks and update PR. |
|
But combining a bug fix with a change to a developer experience ( We should definitely find an agreement what suits best for all devs in the team, and I think it's better to address this in a separate issue / pull request. |
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
Bug Fixes