Programming exercises: User can resolve pending conflict in Problem Statement editor after network interruptions is resolved#12529
Open
Elfari1028 wants to merge 4 commits intodevelopfrom
Conversation
…iff view Adds a reconnection handler that detects when a user has unsaved local changes after a WebSocket reconnection. When the local snapshot differs from the newly synced content, the editor switches to diff mode showing the user's unsaved changes on the left and the latest synced version on the right, allowing selective application of changes. - Add `reconnected$` observable to `ExerciseEditorSyncService` that emits on
github-actions
bot
added
client
|
@Elfari1028 Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
Contributor
WalkthroughAdds a reconnection observable and UI flow: on websocket reconnect after a prior connection, the editor snapshots local content, re-syncs server content, compares them, and conditionally shows a diff view with a dismiss action. Changes
Sequence DiagramsequenceDiagram
participant WS as WebSocket
participant SyncService as ExerciseEditorSyncService
participant Editor as ProgrammingExerciseEditableInstructionComponent
participant ProblemSync as ProblemStatementSyncService
participant Monaco as MonacoEditorComponent
WS->>SyncService: connectionState$ changes disconnected → connected
SyncService->>Editor: reconnected$ emits
Editor->>Editor: handleReconnection(exerciseId)
Editor->>Editor: Snapshot local editor content
Editor->>ProblemSync: tear down & re-init sync for exerciseId
ProblemSync->>Editor: initialSyncFinalized$ emits (final server content)
Editor->>Editor: Compare snapshot vs final server content
alt content diverged
Editor->>Editor: set reconnection labels & reconnectionDiffActive = true
Editor->>Monaco: setDiffOriginalContent(snapshot)
Monaco->>Monaco: update left/original diff pane
Editor->>Monaco: render diff (mode='diff') with dismiss button
else content identical
Editor->>Editor: dismissReconnectionDiff() (no diff shown)
end
Note over Editor,Monaco: User clicks "Done" → dismissReconnectionDiff()
Editor->>Editor: reconnectionDiffActive = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/webapp/app/exercise/synchronization/services/exercise-editor-sync.service.spec.ts`:
- Around line 472-518: Add a regression test to ensure the initial successful
connection isn't treated as a reconnect: in the reconnected$ spec, create a new
it() that subscribes to service.reconnected$, then push
connectionState$.next(new ConnectionState(false, false)) followed by
connectionState$.next(new ConnectionState(true, true)) and assert that the
subscription callback was not called (emit count remains 0); reference the
existing reconnected$ observable and the ConnectionState constructor to locate
where to add the test.
In
`@src/main/webapp/app/exercise/synchronization/services/exercise-editor-sync.service.ts`:
- Around line 236-254: The reconnected$ getter emits on the very first
false→true transition because it only checks the immediate pair; to restrict it
to true reconnects, add a service-level boolean flag (e.g., private
everConnected: boolean = false) that is set to true whenever
websocketService.connectionState emits connected === true, and then update
reconnected$'s filter to require everConnected to be true in addition to the
pairwise check (i.e., filter(([wasConnected, isConnected]) => everConnected &&
!wasConnected && isConnected)). Ensure everConnected is updated from
websocketService.connectionState subscriptions so the getter remains a pure
Observable creation.
In
`@src/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts`:
- Around line 629-632: When the initialSyncFinalized$ subscriber sees snapshot
=== finalContent it currently returns early but fails to clear reconnection
state; update the callback in
programming-exercise-editable-instruction.component.ts (the subscription to
problemStatementSyncService.initialSyncFinalized$) to reset
reconnectionDiffActive and any override label/state (the component properties
managing the "override" labels and reconnection view) before returning so the
stale diff view and labels are cleared when there is no conflict.
- Around line 628-644: The one-off subscription to initialSyncFinalized$ (the
take(1) call) can outlive the reconnect that created it and consume a later
finalize event with the wrong snapshot; store the Subscription returned by
subscribe (e.g., this.initialSyncFinalizeSub) when you call
initialSyncFinalized$.pipe(take(1)).subscribe(...), unsubscribe that
Subscription before starting a new reconnect cycle (and set it to undefined),
and also unsubscribe it in ngOnDestroy to ensure stale callbacks cannot reopen
the diff with outdated data; keep the existing logic inside the subscription
(including calling this.markdownEditorMonaco?.setDiffOriginalContent(snapshot)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8720204-8ceb-4ead-93e2-3d5bbae9ea17
📒 Files selected for processing (8)
src/main/webapp/app/exercise/synchronization/services/exercise-editor-sync.service.spec.tssrc/main/webapp/app/exercise/synchronization/services/exercise-editor-sync.service.tssrc/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.htmlsrc/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.tssrc/main/webapp/i18n/de/programmingExercise.jsonsrc/main/webapp/i18n/en/programmingExercise.json
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
Claudia-Anthropica
requested changes
Apr 14, 2026
Contributor
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@Elfari1028 Nice approach overall — the reconnection flow with snapshot-compare-diff is well thought out and the tests cover the new observable nicely. Two medium issues: the reconnected$ getter recreates a stateful pairwise() chain on every access, and the take(1) subscription inside handleReconnection is untracked, which can leak or double-fire on rapid reconnections. See inline comments.
End-to-End Test Results
Test Strategy: Two-phase execution
Overall: ❌ Phase 1 (relevant tests) failed 🔗 Workflow Run · 📊 Test Report |
…nections and prevent memory leaks - Change `reconnected$` from getter to readonly property to avoid creating multiple pairwise buffers - Filter out initial connection by checking `wasEverConnectedBefore` flag - Add test case verifying no emission on initial connect - Add `reconnectionFinalizeSubscription` to properly clean up subscription in teardown - Dismiss reconnection diff when snapshot matches final content after sync
Claudia-Anthropica
previously approved these changes
Apr 14, 2026
Contributor
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@Elfari1028 Both medium issues are addressed — reconnected$ is now a proper readonly field and the finalize subscription is tracked and cleaned up. Nice work on the reconnection conflict flow. Approving.
|
@Elfari1028 Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
…tatement-editor-on-reconnection
|
@Elfari1028 Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
…mming exercise update component spec
Elfari1028
dismissed stale reviews from coderabbitai[bot] and Claudia-Anthropica
via
d58951a
|
@Elfari1028 Test coverage has been automatically updated in the PR description. |
|
@Elfari1028 Your PR description needs attention before it can be reviewed: Issues Found
How to Fix
This check validates that your PR description follows the PR template. A complete description helps reviewers understand your changes and speeds up the review process.
|
Claudia-Anthropica
approved these changes
Apr 20, 2026
Contributor
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@Elfari1028 Both medium issues from my previous review are properly fixed — reconnected$ is a readonly field now and the finalize subscription is tracked and cleaned up. The reconnection flow is solid: snapshot → teardown → re-init → compare → diff is clean and all the edge cases (no conflict, rapid reconnections, component destruction) are handled correctly. Nice work.
HawKhiem
approved these changes
Apr 20, 2026
Contributor
HawKhiem
left a comment
HawKhiem
left a comment
There was a problem hiding this comment.
Code lgtm 👍. Will test on a test server tonight
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restore problem statement synchronization cleanly after WebSocket reconnects and surface unsaved-vs-latest conflicts in a dedicated diff view.
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
Problem statement editing uses collaborative synchronization. After a temporary WebSocket disconnect, the local editor could remain out of sync with the latest shared state, and users had no guided way to compare their unsaved local text against the synchronized version after reconnecting.
Description
This PR adds reconnection handling for the programming exercise problem statement editor.
When the exercise editor detects a WebSocket reconnect, it snapshots the current local problem statement, tears down the Yjs binding, and initializes synchronization again from the current exercise state. After the initial sync finalizes, the component compares the locally snapshotted text with the newly synchronized content.
If both versions differ, the editor automatically switches into diff mode:
To support this, the PR introduces:
reconnected$stream inExerciseEditorSyncServiceProgrammingExerciseEditableInstructionComponentMarkdownEditorMonacoComponentThe PR also adds unit tests for the new reconnect detection stream.
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Client
Last updated: 2026-04-20 08:02:17 UTC
Screenshots
Summary by CodeRabbit
New Features
Tests