-
Notifications
You must be signed in to change notification settings - Fork 358
Adaptive learning: Allow instructors to clear Companion's chat and context
#12027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Adaptive learning: Allow instructors to clear Companion's chat and context
#12027
Conversation
|
@Yhmidi Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
WalkthroughAdds a backend clear-session API and service integration to delete chat memory and evict pending operations, a frontend "New Chat" UI action that calls it and resets the chat, plus repository, tests, and i18n keys. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Agent Chat Modal"
participant Client as "AgentChatService"
participant API as "AtlasAgentResource"
participant Service as "AtlasAgentService"
participant Repo as "AtlasChatMemoryRepository"
participant Cache as "PendingOperationsCache"
User->>UI: Click "New Chat"
UI->>UI: check isAgentTyping / isClearingSession
UI->>Client: clearSession(courseId)
Client->>API: POST /api/atlas/agent/courses/{courseId}/chat/clear-session
API->>Service: clearSession(sessionId)
Service->>Repo: deleteByConversationId(sessionId)
Service->>Cache: evict(sessionId)
Repo-->>Service: deleted
Cache-->>Service: evicted
Service-->>API: 200 OK
API-->>Client: 200 OK
Client-->>UI: observable resolved
UI->>UI: reset messages, inject welcome, focus input
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.html`:
- Around line 3-14: Replace the hard-coded "New Chat" button label and the title
tooltip with translatable keys: update the template for the button in
agent-chat-modal.component.html that uses plusIcon and calls
onNewChat()/isAgentTyping()/closeModal() to use jhiTranslate (or the project's
i18n translation directive/pipe) for both the visible text and the title
attribute (e.g. add a key like artemisApp.agent.chat.new and
artemisApp.agent.chat.newTooltip), and ensure the component's translation keys
are added to the i18n resource files; keep the existing (click)="onNewChat()"
and [disabled]="isAgentTyping()" behavior intact.
In `@src/main/webapp/app/atlas/manage/services/agent-chat.service.ts`:
- Around line 69-79: The clearSession method wraps the HTTP POST in a catchError
that swallows failures; remove the catchError block so the Observable returned
by clearSession(courseId: number) is the raw this.http.post<void>(...) and
errors propagate to callers (e.g., onNewChat in agent-chat-modal.component) so
the UI error handler can run; update clearSession by returning the HTTP
observable directly and ensure callers handle errors as needed.
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/AtlasAgentService.java (1)
26-26: Avoid@Transactionalfor cache-onlyclearSession.
clearSessiononly clears chat memory and cache state; wrapping it in a transaction adds overhead without clear benefit. Consider removing it unless there is a concrete transactional requirement.♻️ Suggested change
-import org.springframework.transaction.annotation.Transactional;- `@Transactional` public void clearSession(String sessionId) { if (chatMemory != null) chatMemory.clear(sessionId); clearCachedPendingCompetencyOperations(sessionId); }As per coding guidelines, avoid
@Transactionalscope.Also applies to: 502-508
src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.html
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
added tests
|
@Yhmidi Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/webapp/i18n/de/agent.json`:
- Line 5: Replace the incorrect German label for the JSON key "newChatTitle":
update its value from "Neu Chat" to a grammatically correct alternative such as
"Neuer Chat" (or "Neues Gespräch") so the UI shows proper German; locate the
"newChatTitle" entry in src/main/webapp/i18n/de/agent.json and change the string
value accordingly.
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/atlas/agent/AtlasAgentIntegrationTest.java (1)
344-358: Consider adding a tutor authorization test for consistency.The existing test groups in this file (
ConversationHistoryandAuthorization) test forbidden access for both students and tutors. For consistency and complete access control coverage, consider adding a test for tutor access:♻️ Suggested addition
`@Test` `@WithMockUser`(username = TEST_PREFIX + "student1", roles = "USER") void shouldReturnForbiddenForNonInstructor() throws Exception { request.performMvcRequest(post("/api/atlas/agent/courses/{courseId}/chat/clear-session", course.getId())).andExpect(status().isForbidden()); } + + `@Test` + `@WithMockUser`(username = TEST_PREFIX + "tutor1", roles = "TA") + void shouldReturnForbiddenForTutor() throws Exception { + request.performMvcRequest(post("/api/atlas/agent/courses/{courseId}/chat/clear-session", course.getId())).andExpect(status().isForbidden()); + } }
|
@Yhmidi Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
|
@Yhmidi Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
|
@Yhmidi Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
florian-glombik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Request
- Please add the translation for the button
- Please get rid of the magic number, ideally extract a more readable method
Other Comments
Optional
src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.html
Show resolved
Hide resolved
src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.ts
Show resolved
Hide resolved
src/main/webapp/app/atlas/manage/agent-chat-modal/agent-chat-modal.component.ts
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/atlas/agent/AtlasAgentIntegrationTest.java
Show resolved
Hide resolved
src/main/webapp/app/atlas/manage/services/agent-chat.service.spec.ts
Outdated
Show resolved
Hide resolved
- added string key - added integration test - preventing race conditions by adding a flag to when the new chat is clicked
|
@Yhmidi Test coverage has been automatically updated in the PR description. |
1 similar comment
|
@Yhmidi Test coverage has been automatically updated in the PR description. |
MarcosOlivaKaczmarek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3, it does not forget about the context in the new chat.
Grabacion.de.pantalla.2026-01-25.a.las.18.06.58.mov
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…able to use @transactional as it is necessary for our clearSession method as ChatMemoryRepository.deleteByConversationId(sessionId); doest it handle it on its own
…able to use @transactional as it is necessary for our clearSession method as ChatMemoryRepository.deleteByConversationId(sessionId); doest it handle it on its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/de/tum/cit/aet/artemis/atlas/repository/AtlasChatMemoryRepository.java`:
- Around line 23-36: In AtlasChatMemoryRepository.deleteByConversationId, add
input validation for the conversationId parameter before calling
chatMemoryRepository: check for null or blank (e.g., null or
conversationId.trim().isEmpty()) and short-circuit (return immediately) to avoid
forwarding invalid IDs to chatMemoryRepository; if desired, log a warning or
throw an IllegalArgumentException for clearly invalid callers, but do not call
chatMemoryRepository.deleteByConversationId when the ID is invalid.
In `@src/main/java/de/tum/cit/aet/artemis/atlas/service/AtlasAgentService.java`:
- Around line 505-508: In clearSession, guard against a null
atlasChatMemoryRepository and invalid sessionId by validating/sanitizing
sessionId (non-null, non-empty) before using it, and only call
atlasChatMemoryRepository.deleteByConversationId(sessionId) when
atlasChatMemoryRepository != null and sessionId is valid; likewise call
clearCachedPendingCompetencyOperations(sessionId) only after the same sessionId
validation to avoid NPEs and invalid-cache operations.
| public AtlasChatMemoryRepository(@Autowired(required = false) ChatMemoryRepository chatMemoryRepository) { | ||
| this.chatMemoryRepository = chatMemoryRepository; | ||
| } | ||
|
|
||
| /** | ||
| * Deletes all messages for a given conversation/session. | ||
| * | ||
| * @param conversationId the conversation/session ID | ||
| */ | ||
| @Transactional | ||
| public void deleteByConversationId(String conversationId) { | ||
| if (chatMemoryRepository != null) { | ||
| chatMemoryRepository.deleteByConversationId(conversationId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate conversationId before deleting.
If conversationId is null/blank, the underlying repository may error or delete unintended data. Add a guard to short-circuit invalid input (Line 33).
✅ Proposed fix
`@Transactional`
public void deleteByConversationId(String conversationId) {
- if (chatMemoryRepository != null) {
+ if (chatMemoryRepository != null && conversationId != null && !conversationId.isBlank()) {
chatMemoryRepository.deleteByConversationId(conversationId);
}
}Based on learnings, validate and sanitize inputs in server-side code.
🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/atlas/repository/AtlasChatMemoryRepository.java`
around lines 23 - 36, In AtlasChatMemoryRepository.deleteByConversationId, add
input validation for the conversationId parameter before calling
chatMemoryRepository: check for null or blank (e.g., null or
conversationId.trim().isEmpty()) and short-circuit (return immediately) to avoid
forwarding invalid IDs to chatMemoryRepository; if desired, log a warning or
throw an IllegalArgumentException for clearly invalid callers, but do not call
chatMemoryRepository.deleteByConversationId when the ID is invalid.
| public void clearSession(String sessionId) { | ||
| atlasChatMemoryRepository.deleteByConversationId(sessionId); | ||
| clearCachedPendingCompetencyOperations(sessionId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against null repository and invalid session IDs.
atlasChatMemoryRepository is optional; calling deleteByConversationId without a null check can NPE. Also validate sessionId before cache or repository operations (Line 505).
✅ Proposed fix
public void clearSession(String sessionId) {
- atlasChatMemoryRepository.deleteByConversationId(sessionId);
- clearCachedPendingCompetencyOperations(sessionId);
+ if (sessionId == null || sessionId.isBlank()) {
+ return;
+ }
+ if (atlasChatMemoryRepository != null) {
+ atlasChatMemoryRepository.deleteByConversationId(sessionId);
+ }
+ clearCachedPendingCompetencyOperations(sessionId);
}Based on learnings, validate and sanitize inputs in server-side code.
📝 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.
| public void clearSession(String sessionId) { | |
| atlasChatMemoryRepository.deleteByConversationId(sessionId); | |
| clearCachedPendingCompetencyOperations(sessionId); | |
| } | |
| public void clearSession(String sessionId) { | |
| if (sessionId == null || sessionId.isBlank()) { | |
| return; | |
| } | |
| if (atlasChatMemoryRepository != null) { | |
| atlasChatMemoryRepository.deleteByConversationId(sessionId); | |
| } | |
| clearCachedPendingCompetencyOperations(sessionId); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/de/tum/cit/aet/artemis/atlas/service/AtlasAgentService.java`
around lines 505 - 508, In clearSession, guard against a null
atlasChatMemoryRepository and invalid sessionId by validating/sanitizing
sessionId (non-null, non-empty) before using it, and only call
atlasChatMemoryRepository.deleteByConversationId(sessionId) when
atlasChatMemoryRepository != null and sessionId is valid; likewise call
clearCachedPendingCompetencyOperations(sessionId) only after the same sessionId
validation to avoid NPEs and invalid-cache operations.
|
@Yhmidi Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
…onversationbyID() need @transactional in order to function correctly . so had to exclude the added wrapper repository from the architecture test.
|
@Yhmidi Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
As it is common while interacting with Agents that it sometimes is stuck in a context and unable to help you with your requests. It is the same for our Atlas Companion which introduces the need for a contexting clearing feature.
Description
Server Side:
Client Side:
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.
Test on TS3 for admin rights or locally
Review Progress
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Client
Server
Last updated: 2026-01-25 22:24:28 UTC
Screenshots
Summary by CodeRabbit
New Features
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.