Development: Support internal stages that are hidden from the user
#11399
Conversation
github-actions
bot
added
server
WalkthroughPyrisStageDTO gained a new boolean component Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BE as Backend (Pyris)
participant WS as WebSocket
participant SVC as IrisChatService
participant UI as ChatStatusBar
BE->>WS: Push payload { stages[] } (some stages may have internal=true)
WS-->>SVC: MESSAGE / STATUS payload
rect rgb(235,245,255)
note right of SVC: New filtering step
SVC->>SVC: filterStages(stages) => stages.filter(stage => !stage.internal)
end
SVC-->>UI: stages (internal filtered out)
UI->>UI: Render progress and stage names from received stages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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
🧹 Nitpick comments (7)
src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.ts (1)
6-7: Default to false to avoid undefined at runtimeSafer default in case older backends omit the field; keeps client filtering stable.
Apply this diff:
- // Internal stages are not shown in the UI and are hidden from the user - internal: boolean; + /** Internal stages are not shown in the UI and are hidden from the user */ + internal = false;src/main/webapp/app/iris/overview/services/iris-chat.service.ts (1)
421-423: Be explicit in the filter to handle missing fieldsUsing an explicit check reads clearer and is resilient if
internalis absent or nullable.Apply this diff:
- private filterStages(stages: IrisStageDTO[]): IrisStageDTO[] { - return stages.filter((stage) => !stage.internal); - } + private filterStages(stages: IrisStageDTO[]): IrisStageDTO[] { + return stages.filter(({ internal }) => internal !== true); + }If you want, I can add a focused unit test for this method (one stage with
internal: trueis filtered out; undefined/false are kept).src/test/java/de/tum/cit/aet/artemis/iris/PyrisFaqIngestionTest.java (2)
198-198: Minor wording nit in test stringsThe message reads awkwardly (“Stage not broke due to error.”). Consider clearer phrasing to aid future debugging.
Apply this diff pattern at the listed lines:
- PyrisStageDTO errorStage = new PyrisStageDTO("error", 1, PyrisStageState.ERROR, "Stage not broke due to error.", false); + PyrisStageDTO errorStage = new PyrisStageDTO("error", 1, PyrisStageState.ERROR, "Stage failed due to an error.", false);Also applies to: 211-211, 225-225, 241-241
142-142: Reduce repetition with a small factory helperOptional: a local helper improves readability and keeps the
internaldefault in one place.Add inside the test class:
private static PyrisStageDTO stage(String name, int weight, PyrisStageState state, String message) { return new PyrisStageDTO(name, weight, state, message, false); }Then replace e.g.:
- PyrisStageDTO doneStage = new PyrisStageDTO("done", 1, PyrisStageState.DONE, "Stage completed successfully.", false); + PyrisStageDTO doneStage = stage("done", 1, PyrisStageState.DONE, "Stage completed successfully.");Also applies to: 155-155, 168-169, 183-184
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (1)
125-127: Avoid null state/message in stage templates (optional)Consider small helpers/factories to avoid passing null and to centralize default weights/names for “Preparing” and “Executing pipeline”.
Apply (example):
- var preparing = new PyrisStageDTO("Preparing", 10, null, null, false); - var executing = new PyrisStageDTO("Executing pipeline", 30, null, null, false); + var preparing = new PyrisStageDTO("Preparing", 10, PyrisStageState.NOT_STARTED, null, false); + var executing = new PyrisStageDTO("Executing pipeline", 30, PyrisStageState.NOT_STARTED, null, false);Or introduce constants/factories to dedupe literals used across tests.
src/test/java/de/tum/cit/aet/artemis/iris/MemirisIntegrationTest.java (1)
134-135: Use existing static imports for PyrisStageStateYou already import IN_PROGRESS/DONE statically; drop the FQNs for consistency/readability.
Apply:
- var executingInProgress = new PyrisStageDTO("Executing pipeline", 30, de.tum.cit.aet.artemis.iris.service.pyris.dto.status.PyrisStageState.IN_PROGRESS, null, false); - var executingDone = new PyrisStageDTO("Executing pipeline", 30, de.tum.cit.aet.artemis.iris.service.pyris.dto.status.PyrisStageState.DONE, null, false); + var executingInProgress = new PyrisStageDTO("Executing pipeline", 30, IN_PROGRESS, null, false); + var executingDone = new PyrisStageDTO("Executing pipeline", 30, DONE, null, false);- var executingInProgress = new PyrisStageDTO("Executing pipeline", 30, de.tum.cit.aet.artemis.iris.service.pyris.dto.status.PyrisStageState.IN_PROGRESS, null, false); + var executingInProgress = new PyrisStageDTO("Executing pipeline", 30, IN_PROGRESS, null, false);Also applies to: 194-194
src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (1)
233-233: Polish error message grammar (nitpick)“Stage not broke due to error.” → “Stage failed due to an error.”
Apply:
- PyrisStageDTO errorStage = new PyrisStageDTO("error", 1, PyrisStageState.ERROR, "Stage not broke due to error.", false); + PyrisStageDTO errorStage = new PyrisStageDTO("error", 1, PyrisStageState.ERROR, "Stage failed due to an error.", false);Also applies to: 247-247, 263-263, 279-279
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/status/PyrisStageDTO.java(1 hunks)src/main/webapp/app/iris/overview/base-chatbot/chat-status-bar/chat-status-bar.component.spec.ts(2 hunks)src/main/webapp/app/iris/overview/services/iris-chat.service.ts(1 hunks)src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/MemirisIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisFaqIngestionTest.java(8 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java(10 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
Files:
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/MemirisIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisFaqIngestionTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.tssrc/main/webapp/app/iris/overview/services/iris-chat.service.tssrc/main/webapp/app/iris/overview/base-chatbot/chat-status-bar/chat-status-bar.component.spec.ts
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/status/PyrisStageDTO.java
🧠 Learnings (2)
📚 Learning: 2024-10-15T11:33:17.915Z
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/MemirisIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisFaqIngestionTest.javasrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java
🧬 Code graph analysis (2)
src/main/webapp/app/iris/overview/services/iris-chat.service.ts (1)
src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.ts (1)
IrisStageDTO(1-10)
src/main/webapp/app/iris/overview/base-chatbot/chat-status-bar/chat-status-bar.component.spec.ts (1)
src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.ts (1)
IrisStageDTO(1-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
src/main/webapp/app/iris/overview/base-chatbot/chat-status-bar/chat-status-bar.component.spec.ts (1)
26-26: LGTM — test data updated to new DTO shapeAll updated stage literals now include
internal: falseand keep expectations intact.Also applies to: 34-34, 42-42, 49-49, 54-54, 68-68
src/main/webapp/app/iris/overview/services/iris-chat.service.ts (1)
409-414: LGTM — emitting only non‑internal stages on WS updatesFiltering on both MESSAGE and STATUS payloads matches the feature intent.
src/test/java/de/tum/cit/aet/artemis/iris/IrisTutorSuggestionIntegrationTest.java (1)
309-309: LGTM — constructor updated to new APISupplying the
internalflag (false) aligns with the new 5‑argPyrisStageDTOsignature.src/test/java/de/tum/cit/aet/artemis/iris/PyrisFaqIngestionTest.java (1)
142-142: LGTM — all stage constructions migrated to 5‑arg ctorConsistent use of
falseforinternalacross tests.Also applies to: 155-155, 168-169, 183-184, 198-198, 211-211, 225-225, 241-241
src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1)
67-67: 5‑arg PyrisStageDTO usage — LGTMConstructor update looks correct and aligns with the new API.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java (1)
65-65: 5‑arg PyrisStageDTO usage — LGTMCorrect adaptation to the new signature.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (2)
133-133: 5‑arg PyrisStageDTO in doneStage — LGTM
185-185: 5‑arg PyrisStageDTO in failedStages — LGTMsrc/test/java/de/tum/cit/aet/artemis/iris/MemirisIntegrationTest.java (2)
356-356: Deletion ingestion flow: stage construction — LGTMCorrect 5‑arg usage; semantics unchanged.
385-385: Status text: OKMessage reads naturally; consistent with prior “Stage completed successfully.”
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/status/PyrisStageDTO.java (2)
7-7: New ‘internal’ flag on stage — verify backward compatibilityChange looks right and NON_EMPTY ensures false isn’t serialized. Please confirm legacy payloads (without “internal”) still deserialize to false with records.
You can add a focused test:
// e.g., PyrisStageDTOJsonTest.java @Test void deserializesWithoutInternal_defaultsToFalse() throws Exception { var json = """ {"name":"X","weight":1,"state":"DONE","message":"m"} """; var dto = new ObjectMapper().readValue(json, PyrisStageDTO.class); assertThat(dto.internal()).isFalse(); }
10-27: State-transition helpers preserve ‘internal’ — LGTMAll helpers correctly propagate the new flag.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisLectureIngestionTest.java (1)
171-171: 5‑arg PyrisStageDTO usages — LGTMAll instantiations were updated consistently; no behavior change.
Also applies to: 186-186, 200-201, 216-217, 233-233, 247-247, 263-263, 279-279, 356-356
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
toukhi
left a comment
toukhi
left a comment
There was a problem hiding this comment.
Code looks good to me. Tested locally and works as expected.
hakanduranC
left a comment
hakanduranC
left a comment
There was a problem hiding this comment.
The code review has been completed and approved.
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Nayer-kotry
left a comment
Nayer-kotry
left a comment
There was a problem hiding this comment.
Tested on the iris test server, works as intended.
Hialus
dismissed stale reviews from Nayer-kotry, Anishyou, toukhi, and hakanduranC
via
a6c3419
# Conflicts: # src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
# Conflicts: # src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts(2 hunks)src/main/webapp/app/iris/overview/services/iris-chat.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/iris/overview/services/iris-chat.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
📚 Learning: 2024-10-15T11:33:17.915Z
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Applied to files:
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: RY997
PR: ls1intum/Artemis#7695
File: src/main/webapp/app/iris/exercise-chatbot/widget/chatbot-widget.component.ts:775-777
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Step error messages in the `notifyStepFailed` method of the `IrisChatbotWidgetComponent` are intentionally not dispatched to the state store as part of the changes in PR 7695.
Applied to files:
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
📚 Learning: 2024-10-10T11:42:23.069Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9443
File: src/test/javascript/spec/component/hestia/git-diff-report/git-diff-modal.component.spec.ts:55-60
Timestamp: 2024-10-10T11:42:23.069Z
Learning: In `git-diff-report-modal.component.spec.ts`, using `fakeAsync` and `tick` does not work for handling asynchronous operations in the tests; alternative methods are needed.
Applied to files:
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
📚 Learning: 2025-04-01T17:19:55.677Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#10610
File: src/test/javascript/spec/service/guided-tour.service.spec.ts:193-193
Timestamp: 2025-04-01T17:19:55.677Z
Learning: In the guide tour service tests, the `router.navigateByUrl` mock should remain synchronous (returning boolean) rather than returning a Promise to maintain compatibility with existing test logic that depends on synchronous behavior.
Applied to files:
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts
🧬 Code graph analysis (1)
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts (2)
src/test/javascript/spec/helpers/sample/iris-sample-data.ts (2)
mockServerSessionHttpResponseWithId(151-160)mockWebsocketStatusMessageWithInteralStage(92-105)src/main/webapp/app/iris/shared/entities/iris-stage-dto.model.ts (1)
IrisStageDTO(1-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (1)
src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts (1)
242-252: Well-structured test for internal stage filtering.The test correctly verifies that internal stages are filtered from the
currentStages()observable, aligning with the PR objective to hide internal stages from the UI. The test follows existing patterns, uses appropriate mocking, and the assertion accurately validates the filtering behavior.
| mockUserMessageWithContent, | ||
| mockWebsocketServerMessage, | ||
| mockWebsocketStatusMessage, | ||
| mockWebsocketStatusMessageWithInteralStage, |
There was a problem hiding this comment.
Fix typo in mock variable name.
The variable mockWebsocketStatusMessageWithInteralStage has a typo: "Interal" should be "Internal". This originates from the source file test/helpers/sample/iris-sample-data.ts (line 91) and should be corrected there.
Consider renaming in the source file:
// In test/helpers/sample/iris-sample-data.ts
-export const mockWebsocketStatusMessageWithInteralStage = {
+export const mockWebsocketStatusMessageWithInternalStage = {Then update the import and usage in this file accordingly.
🤖 Prompt for AI Agents
In src/main/webapp/app/iris/overview/services/iris-chat.service.spec.ts around
line 25, the imported mock variable name has a typo
(mockWebsocketStatusMessageWithInteralStage); update the source definition in
test/helpers/sample/iris-sample-data.ts (line ~91) to rename the export to
mockWebsocketStatusMessageWithInternalStage, then update the import in this spec
file to use the corrected name and update any usages accordingly so the
import/export names match.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
bassner
changed the title
Iris: Support internal stages that are hidden in the clientIris: Support internal stages that are hidden from the user
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
krusche
changed the title
Iris: Support internal stages that are hidden from the userDevelopment: Support internal stages that are hidden from the user
Checklist
General
Server
Client
Motivation and Context
We want to hide the long memory creation phase from the user and allow them to chat while it is running.
Description
Soft-linked to ls1intum/edutelligence#260.
I added an internal marker for stages. The UI filters out these stages and does not show them to the user, while still getting info from them.
Steps for Testing
Prerequisites:
Iris: Mark the Memiris stage as internal edutelligence#260 deployed in the iris test serverReview Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
Tests