Iris: Support internal stages that are hidden in the client
#11399
Conversation
github-actions
bot
added
server
WalkthroughA new boolean field internal was added to PyrisStageDTO (backend and frontend), updating its constructor/signature and propagators. Backend call sites and multiple Java tests were adapted to pass the new flag. Frontend now exposes internal on IrisStageDTO and filters out internal stages in IrisChatService. Angular tests updated accordingly. 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 stages [may include internal=true]
WS-->>SVC: MESSAGE / STATUS payload { stages[] }
rect rgb(235, 245, 255)
note right of SVC: New behavior
SVC->>SVC: filterStages(stages) where !stage.internal
end
SVC-->>UI: stages (internal filtered out)
UI->>UI: Render status bar/progress from non-internal stages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. |
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