Communication: Add recents section to sidebar
#10033
Conversation
asliayk
added
tests
client
WalkthroughThis pull request introduces enhancements to the course conversations and sidebar functionality in an Angular application. The changes focus on improving the rendering logic for conversations, adding a new "recents" channel type, and refining the sidebar component's event handling. Key modifications include updating the conversation grouping mechanism to incorporate course context, adding localization for the "recents" label, and simplifying the sidebar's input properties. The overall structure of the component and service remains intact, while the changes aim to provide a more intuitive and context-aware user experience for course-related conversations. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Nitpick comments (4)
src/main/webapp/app/overview/course-overview.service.ts (3)
174-193: Consider renaming 'getConversationGroup' to 'getConversationGroups'Since the method now returns an array of
ChannelGroupCategory, renaming it togetConversationGroupswould improve readability and accurately reflect its functionality.
230-230: Add JSDoc comments for the updated method signatureConsider adding or updating the documentation for
groupConversationsByChannelTypeto reflect the newcourseparameter, explaining its purpose and usage.
294-294: Add JSDoc comments for the updated method signatureConsider adding or updating the documentation for
mapConversationsToSidebarCardElementsto reflect the newcourseparameter, explaining its purpose and usage.src/main/webapp/app/types/sidebar.ts (1)
139-139: Add documentation for 'isCurrent' propertyConsider adding a JSDoc comment to the
isCurrentproperty in theSidebarCardElementinterface to explain its purpose, enhancing code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/webapp/app/overview/course-conversations/course-conversations.component.html(0 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(7 hunks)src/main/webapp/app/overview/course-overview.service.ts(7 hunks)src/main/webapp/app/shared/sidebar/sidebar.component.ts(1 hunks)src/main/webapp/app/types/sidebar.ts(2 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/javascript/spec/component/course/course-overview.service.spec.ts(3 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/i18n/en/student-dashboard.json
👮 Files not reviewed due to content moderation or server errors (3)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
- src/test/javascript/spec/component/course/course-overview.service.spec.ts
- src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/i18n/de/student-dashboard.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/app/types/sidebar.ts (1)
src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/course/course-overview.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/overview/course-overview.service.ts (1)
🔇 Additional comments (5)
src/main/webapp/app/overview/course-overview.service.ts (3)
23-23: Import of 'Course' is appropriate
The import of Course is necessary for the updated method signatures and is correctly added.
62-62: Addition of 'recently' group is appropriate
The inclusion of the recently group in DEFAULT_CHANNEL_GROUPS correctly aligns with the new functionality and is properly initialized.
265-267: Ensure all group names are valid keys in 'groupedConversationGroups'
Verify that all group names returned by getConversationGroup are valid keys in groupedConversationGroups to prevent potential runtime errors.
Run the following script to check for any group names not present in groupedConversationGroups:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The group names are safely typed and validated by the TypeScript type system
The code is safe because:
getConversationGroup()only returns validChannelGroupCategoryvalues: 'favoriteChannels', 'hiddenChannels', 'groupChats', and 'directMessages'- These values are all included in the
ChannelGroupCategorytype definition - The
groupedConversationGroupsis initialized fromchannelGroupswhich is typed asAccordionGroups - TypeScript's type system ensures that the group names used as keys are valid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all group names are valid keys in 'groupedConversationGroups'
# Extract all possible group names from 'ChannelGroupCategory'
echo "Possible group names:"
grep -oP "(?<=\| ')[^']+" src/main/webapp/app/types/sidebar.ts
# Extract group names used in 'getConversationGroup' method
echo "Group names used in 'getConversationGroup' method:"
grep -oP "groups\.push\('([^']+)'\)" src/main/webapp/app/overview/course-overview.service.ts | awk -F"'" '{print $2}'
# Compare group names
echo "Group names not present in 'groupedConversationGroups':"
comm -23 \
<(grep -oP "groups\.push\('([^']+)'\)" src/main/webapp/app/overview/course-overview.service.ts | awk -F"'" '{print $2}' | sort | uniq) \
<(grep -oP "(?<=\| ')[^']+" src/main/webapp/app/types/sidebar.ts | sort | uniq)
Length of output: 1155
Script:
#!/bin/bash
# Let's verify this in a different way by checking the actual implementation
# First, let's see how groupedConversationGroups is initialized
echo "Initialization of groupedConversationGroups:"
rg -A 5 "groupedConversationGroups\s*=" src/main/webapp/app/overview/course-overview.service.ts
# Then, let's see the full implementation of getConversationGroup
echo -e "\nImplementation of getConversationGroup:"
ast-grep --pattern 'getConversationGroup($_) {
$$$
}'
# Finally, let's check where conversationGroups comes from
echo -e "\nUsage context of conversationGroups:"
rg -B 5 -A 5 "const group of conversationGroups" src/main/webapp/app/overview/course-overview.service.ts
Length of output: 1220
Script:
#!/bin/bash
# Let's check the initialization of channelGroups and the getConversationGroup method
# Check where channelGroups is defined/initialized
echo "channelGroups initialization:"
rg -B 2 -A 2 "channelGroups\s*=" src/main/webapp/app/overview/course-overview.service.ts
# Get the full getConversationGroup method implementation
echo -e "\ngetConversationGroup method:"
rg -B 2 -A 15 "getConversationGroup.*\{" src/main/webapp/app/overview/course-overview.service.ts
# Check the type definition of channelGroups
echo -e "\nType definitions:"
rg "type.*AccordionGroups" src/main/webapp/app/types/sidebar.ts -A 5
Length of output: 1744
Script:
#!/bin/bash
# Let's check the DEFAULT_CHANNEL_GROUPS definition and ChannelGroupCategory type
# Check DEFAULT_CHANNEL_GROUPS initialization
echo "DEFAULT_CHANNEL_GROUPS definition:"
rg "DEFAULT_CHANNEL_GROUPS\s*=" -A 10 src/main/webapp/app/overview/course-overview.service.ts
# Get the complete ChannelGroupCategory type definition
echo -e "\nComplete ChannelGroupCategory definition:"
rg "ChannelGroupCategory.*=" -A 15 src/main/webapp/app/types/sidebar.ts
Length of output: 930
src/main/webapp/app/types/sidebar.ts (1)
19-19: Addition of 'recently' to 'ChannelGroupCategory'
The inclusion of 'recently' in the ChannelGroupCategory type is appropriate and aligns with the new functionality.
src/main/webapp/i18n/de/student-dashboard.json (1)
85-85: Translation added correctly with appropriate informal language
The key "recently": "Kürzlich" is added correctly and follows the guideline of using informal language (duzen) in German translations.
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/webapp/app/overview/course-overview.service.ts (1)
Line range hint
232-278: Consider adding parameter validation.While the implementation correctly handles favorite sorting and group organization, consider adding validation for the
courseparameter to prevent potential runtime errors.groupConversationsByChannelType(course: Course, conversations: ConversationDTO[], messagingEnabled: boolean): AccordionGroups { + if (!course) { + throw new Error('Course parameter is required for conversation grouping'); + } const channelGroups = messagingEnabled ? { ...DEFAULT_CHANNEL_GROUPS, groupChats: { entityData: [] }, directMessages: { entityData: [] } } : DEFAULT_CHANNEL_GROUPS;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/overview/course-overview.service.ts(7 hunks)src/test/javascript/spec/component/course/course-overview.service.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/course/course-overview.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/overview/course-overview.service.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/overview/course-overview.service.ts (2)
62-62: LGTM: Addition of 'recently' group aligns with requirements.
The addition of the 'recently' group to DEFAULT_CHANNEL_GROUPS is consistent with the PR objective to add a "Recently" section to the sidebar.
174-195: LGTM: Well-structured implementation of multi-group support.
The refactored method now correctly supports conversations appearing in multiple groups (e.g., both in favorites and their original category) while maintaining a clear precedence order:
- Hidden channels
- Favorite channels
- Type-based categorization
src/test/javascript/spec/component/course/course-overview.service.spec.ts (1)
483-528: LGTM: Comprehensive test coverage for new functionality.
The test suite effectively covers all key aspects of the new functionality:
- Favorite conversation handling (lines 483-498)
- Date-based filtering for the "Recently" section (lines 500-528)
The test cases are well-structured and provide good coverage of edge cases.
eylulnc
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
added
deploy:artemis-test1
deploy:artemis-test3
and removed
deploy:artemis-test3
deploy:artemis-test1
labels
…or-recent-channels # Conflicts: # src/main/webapp/i18n/de/student-dashboard.json # src/main/webapp/i18n/en/student-dashboard.json
asliayk
dismissed stale reviews from anian03, kevinfischer4, sachmii, and coderabbitai[bot]
via
abb1be9
…or-recent-channels
asliayk
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
404-407: Consider optimizing the recents filtering logicThe implementation correctly adds recent conversations to the sidebar, but there are potential improvements:
- this.sidebarConversations = this.courseOverviewService.mapConversationsToSidebarCardElements(this.course!, this.conversationsOfUser); - this.accordionConversationGroups = this.courseOverviewService.groupConversationsByChannelType(this.course!, this.conversationsOfUser, this.messagingEnabled); - const currentConversations = this.sidebarConversations?.filter((item) => item.isCurrent) || []; - this.accordionConversationGroups.recents.entityData = currentConversations; + this.sidebarConversations = this.courseOverviewService.mapConversationsToSidebarCardElements(this.course!, this.conversationsOfUser); + this.accordionConversationGroups = this.courseOverviewService.groupConversationsByChannelType( + this.course!, + this.conversationsOfUser, + this.messagingEnabled, + this.sidebarConversations.filter((item) => item.isCurrent) + );This would:
- Move the filtering logic to the service layer for better separation of concerns
- Reduce memory allocation by avoiding temporary array
- Improve type safety by removing optional chaining
src/main/webapp/app/shared/sidebar/sidebar.component.ts (3)
Line range hint
108-127: Enhance error handling and clean up commented codeThe subscription logic could be improved:
- Add error handling for the subscription
- Remove the commented-out
switchMapoperator- Consider using
takeUntilpattern for cleaner unsubscriptionprivate subscribeToSidebarEvents() { this.sidebarEventSubscription?.unsubscribe(); const listener = this.sidebarEventService.sidebarCardEventListener(); let pipe; if (this.reEmitNonDistinctSidebarEvents()) { pipe = listener; } else { pipe = listener.pipe( distinctUntilChanged(), - // switchMap(sidebarCardEvent => sidebarCardEvent.onEvent), ); } this.sidebarEventSubscription = pipe.subscribe( + { + next: (itemId) => { + if (itemId) { + this.storeLastSelectedItem(itemId); + if (this.sidebarData.sidebarType == 'conversation') { + this.onSelectConversation.emit(itemId); + this.onUpdateSidebar.emit(); + } + } + }, + error: (error) => { + console.error('Error in sidebar events subscription:', error); + } + } ); }
Line range hint
209-211: Improve method naming for clarityThe method name
markAllMessagesAsCheckedcould be more descriptive in the context of channel communication.-markAllMessagesAsChecked() { +markAllChannelsAsRead() { this.onMarkAllChannelsAsRead.emit(); }
Line range hint
22-211: Consider splitting component responsibilitiesThe component currently handles multiple distinct features (exercises, conversations, exams) and contains complex filtering logic. Consider breaking it down into smaller, more focused components:
- Create separate components for each sidebar type (exercise, conversation, exam)
- Extract filtering logic into a dedicated service
- Use composition to combine these components based on the sidebar type
This would improve maintainability, testability, and follow the Single Responsibility Principle.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/overview/course-conversations/course-conversations.component.html(0 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(7 hunks)src/main/webapp/app/shared/sidebar/sidebar.component.ts(1 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/overview/course-conversations/course-conversations.component.html
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/i18n/de/student-dashboard.json
- src/main/webapp/i18n/en/student-dashboard.json
- src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
16-16: LGTM: Constants and icon additions are consistent!
The additions for the new "Recents" section follow the existing patterns and Angular style guide:
- Icon mapping using
faClock - Default configurations in the constants
Also applies to: 48-48, 67-67, 80-80, 93-93
src/main/webapp/app/shared/sidebar/sidebar.component.ts (2)
6-6: LGTM: Clean type imports
The import statement follows Angular naming conventions and properly groups related types.
Line range hint 29-39: Incorrect usage of input() and output() functions
The current implementation uses non-standard Angular constructs that may cause issues.
This is a duplicate of a previous review comment. Please refer to the existing comment for the recommended fixes using standard Angular @Input() and @Output() decorators with EventEmitter.
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Description
startDateis used as the reference, while for exercises, thedueDateandreleaseDateare considered.ChannelAccordionShowAddhas been removed.Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores