Communication: Allow users to search in specific conversations and from specific authors
#10661
Conversation
…and close button in search bar
github-actions
bot
added
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
cremertim
left a comment
cremertim
left a comment
There was a problem hiding this comment.
Had some remarks on the code during testing session.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/communication/shared/conversation-global-search/conversation-global-search.component.ts (3)
142-163: Consider optimizing the filtering logicThe filtering of conversations happens in two steps: first filtering out already selected conversations, then filtering by search query. These operations could be combined for better performance.
- const notYetSelectedConversations = this.conversations().filter((conversation) => { - return !this.selectedConversations.some((selected) => selected.id === conversation.id); - }); - - let matchingConversations = notYetSelectedConversations; - if (searchQuery) { - matchingConversations = matchingConversations.filter((conversation) => { - const name = this.getConversationName(conversation); - return name.toLowerCase().includes(searchQuery); - }); - } + let matchingConversations = this.conversations().filter((conversation) => { + // Filter out already selected conversations + const isNotSelected = !this.selectedConversations.some((selected) => selected.id === conversation.id); + + // If we have a search query, also filter by name + if (!isNotSelected) return false; + if (!searchQuery) return true; + + const name = this.getConversationName(conversation); + return name.toLowerCase().includes(searchQuery); + });
165-204: Address optional chaining in user filtering conditionsThe static analysis tool flagged potential null/undefined access in the user filtering logic. Use optional chaining to make these checks safer.
- if ( - (this.user && this.user.name?.toLowerCase().includes(searchQuery.toLowerCase())) || - (this.user && this.user.login?.toLowerCase().includes(searchQuery.toLowerCase())) - ) { + if ( + this.user?.name?.toLowerCase().includes(searchQuery.toLowerCase()) || + this.user?.login?.toLowerCase().includes(searchQuery.toLowerCase()) + ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
165-204: Improve error handling in user searchThe error handling in the user search HTTP request is minimal. Consider displaying a user-friendly error message when the request fails.
catchError(() => of([])), + catchError((error) => { + console.error('Error searching users:', error); + this.userSearchStatus = UserSearchStatus.RESULTS; + return of([]); + }), takeUntil(this.destroy$),🧰 Tools
🪛 Biome (1.9.4)
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/communication/shared/conversation-global-search/conversation-global-search.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/communication/shared/conversation-global-search/conversation-global-search.component.ts
🪛 Biome (1.9.4)
src/main/webapp/app/communication/shared/conversation-global-search/conversation-global-search.component.ts
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (6)
src/main/webapp/app/communication/shared/conversation-global-search/conversation-global-search.component.ts (6)
1-19: Well-structured imports and organizationGood job on organizing imports following Angular best practices. The component imports are properly classified by type (Angular core, models, services, etc.) while keeping everything needed for the functionality.
20-48: Good use of interfaces and enumsClean definition of interfaces and enums with descriptive names following PascalCase convention. The search prefixes as constants also make the code more maintainable.
49-93: Component setup follows Angular best practicesGood implementation using Angular's input/output signals and viewChild. The component is properly set up with necessary dependencies and follows the coding guidelines for naming conventions.
94-103: Proper lifecycle managementThe component correctly implements OnInit and OnDestroy interfaces with appropriate cleanup of observables using the destroy$ subject, which prevents memory leaks.
307-333: Well-implemented keyboard navigation with visual focusExcellent implementation of keyboard navigation for the dropdown with proper scrolling to keep the active item visible. This enhances accessibility and user experience.
342-356: Good implementation of keyboard shortcut and click outside handlingThe component properly handles document-level events for closing the dropdown when clicking outside and focusing the search input using Ctrl+K or Cmd+K, which enhances the user experience.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
eylulnc
left a comment
eylulnc
left a comment
There was a problem hiding this comment.
Tested on TS5, work as expected.
HawKhiem
left a comment
HawKhiem
left a comment
There was a problem hiding this comment.
Retested on TS3. The "by:..." doesn't work anymore, the "in:..." still works
Sorry, we forgot to mention: 'by:' has been changed to 'from:' |
HawKhiem
left a comment
HawKhiem
left a comment
There was a problem hiding this comment.
Retested on TS5. Works now
PaRangger
left a comment
PaRangger
left a comment
There was a problem hiding this comment.
Tested on TS5, works very well 😄
cremertim
left a comment
cremertim
left a comment
There was a problem hiding this comment.
code changes lgtm. thanks for incorporating my remarks!
krusche
changed the title
Communication: Add conversation and author selection to search barCommunication: Allow users to search in specific conversations and from specific authors
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
Checklist
General
Client
Motivation and Context
With #10601 we adapted the messages endpoint to allow for searching messages with a specific author or in specific channels. This PR introduces the frontend changes to make this new functionality available in the web UI.
Description
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Screenshots
Test Coverage
Client
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Style
Tests
Documentation