CLUE-341-group-doc-live by scytacki · Pull Request #2753 · concord-consortium/collaborative-learning

scytacki · 2026-02-05T18:08:14Z

Finally this PR adds support for a group document which shows remotes edits as they are made by other users in the group.

To test group documents you have to use a unit with the groupDocumentsEnabled config set. Currently this might only be on the qa unit: unit=unit=./demo/units/qa/content.json.

The history viewer debugging tool, has been updated so it shows more information, and so it can pause the sending of history entries up to Firestore. This makes it possible to deterministically re-create "race condition" issues. The group-documents.md describes some of these issues.

The document content is only loaded once when the document is first opened. After this all changes from remote users come in via history events. Changes to the remote document content in the realtime database are ignored. This is "ignore of changes to remote document document" is the same way normal CLUE documents behave when they are opened for editing. So there is no special code needed to get this behavior when editing a group document.

Known issues with this code are in group-documents.md

The following PR merges in master: #2757
The test that failed in this PR should be fixed in that one. And I'll make sure the rest of the Cypress tests pass in that follow on PR.

codecov · 2026-02-05T18:10:10Z

Codecov Report

❌ Patch coverage is 5.73770% with 115 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.19%. Comparing base (e8382ef) to head (55d021b).
⚠️ Report is 245 commits behind head on group-documents.

Files with missing lines	Patch %	Lines
...ls/history/firestore-history-manager-concurrent.ts	4.85%	98 Missing ⚠️
src/components/document/history-view-panel.tsx	11.11%	16 Missing ⚠️
src/components/document/history-entry-item.tsx	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e8382ef) and HEAD (55d021b). Click for more details.

HEAD has 14 uploads less than BASE

Flag BASE (e8382ef) HEAD (55d021b)

cypress-regression 14 0

Additional details and impacted files

@@                 Coverage Diff                  @@
##           group-documents    #2753       +/-   ##
====================================================
- Coverage            85.78%   66.19%   -19.59%     
====================================================
  Files                  820      815        -5     
  Lines                44039    44110       +71     
  Branches             11255    11272       +17     
====================================================
- Hits                 37778    29200     -8578     
- Misses                6245    14875     +8630     
- Partials                16       35       +19

Flag	Coverage Δ
cypress-regression	`?`
cypress-smoke	`43.75% <0.00%> (-0.11%)`	⬇️
jest	`49.42% <5.73%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cypress · 2026-02-05T18:12:12Z

collaborative-learning Run #17655

Run Properties: Failed #17655 • 760a97d368: CLUE-341 address PR comments

Project	`collaborative-learning`
Branch Review	`CLUE-341-group-doc-live`
Run status	`Failed #17655`
Run duration	`04m 43s`
Commit	`760a97d368: CLUE-341 address PR comments`
Committer	`Scott Cytacki`
View all properties for this run ↗︎

Test results
Failures	`1`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`3`
View all changes introduced in this branch ↗︎

Tests for review

cypress/e2e/smoke/single_student_canvas_test.js • 1 failed test • Smoke tests

View Output

Test		Artifacts
single student functional test > can copy all tiles into workspace and other documents using toolbar helpers		`Test Replay` `Screenshots`

Copilot

Pull request overview

Adds support for collaborative “group documents” by extending Firestore history handling to support concurrent edits, and updates the history debug UI/docs to help reproduce race conditions.

Changes:

Introduces FirestoreHistoryManagerConcurrent with queued transactional uploads, remote history syncing, and pause/resume upload controls.
Updates history viewer UI to show more Firestore metadata (index, previousEntryId) and adds controls for pausing/resuming uploads.
Adds documentation on group document behavior/known issues and adjusts tests accordingly.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/models/mst.test.ts	Adds/organizes MST snapshot tests, including instance reuse in arrays.
src/models/history/tree-manager-history.test.ts	Removes tests related to `setNumHistoryEntriesAppliedFromFirestore`.
src/models/history/firestore-history-manager.test.ts	Updates history loading tests/types to use `HistoryEntrySnapshot` and refines status expectations.
src/models/history/firestore-history-manager-concurrent.ts	Implements concurrent Firestore history manager with queueing, transactions, remote sync, and pause/resume.
src/components/document/history-view-panel.tsx	Enhances remote history display and adds pause/resume controls for concurrent manager.
src/components/document/history-view-panel.scss	Styles new history manager control buttons.
src/components/document/history-entry-item.tsx	Displays Firestore metadata and improves timestamp precision formatting.
src/components/document/canvas.tsx	Minor import ordering change.
docs/group-documents.md	New documentation describing group documents, known issues, and TODOs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/models/history/firestore-history-manager-concurrent.ts

Copilot · 2026-02-06T16:46:59Z

src/models/history/firestore-history-manager-concurrent.ts

+  resumeUploadsAfterDelay(delayMs: number) {
+    setTimeout(() => {
+      runInAction(() => {
+        this.paused = false;
+      });
+      this.uploadQueuedHistoryEntries();
+    }, delayMs);
+  }


resumeUploadsAfterDelay() doesn’t track/cancel outstanding timers. Multiple clicks can schedule multiple timeouts, potentially resuming earlier/later than intended and triggering redundant upload attempts. Consider storing the timeout id (e.g., this.resumeTimeoutId) and clearing it before scheduling a new one, and also handle/record errors from uploadQueuedHistoryEntries() (see unhandled promise risk).

src/models/history/firestore-history-manager-concurrent.ts

src/components/document/history-entry-item.tsx

docs/group-documents.md

most complex change is better error handling in uploadQueuedHistoryEntries. The approach doesn't fully fix the problem, but at least it prevents a messed up uploadInProgress state.

tealefristoe

Looks good 👍

I found one possible issue in firestore-history-manager-concurrent.ts, but otherwise just a few minor suggestions/questions.

tealefristoe · 2026-03-05T03:13:50Z

docs/group-documents.md

+- user B deletes the first object
+- resume user A's uploads
+
+The result of this is that user B will see different object colors than user B.


Suggested change

The result of this is that user B will see different object colors than user B.

The result of this is that user A will see different object colors than user B.

tealefristoe · 2026-03-05T03:14:58Z

docs/group-documents.md

+
+The result of this is that user B will see different object colors than user B.
+
+This behavior should happen for any common drawing object properties: fill color, stroke width, position, rotation, flipping. If there is a property like "text" of a text object that doesn't exist on other objects, this seems to just be ignored. It doesn't show an error in the console.


What happens if you change a property of the last object/delete the last object?

It should behave the same as when the tile is deleted above. If the delete happens first, then when the property change gets applied to the deleted object there will be a MST error about trying to apply a patch to something that doesn't exist. But everything will look OK in the UI.

tealefristoe · 2026-03-05T03:18:24Z

docs/group-documents.md

+- user B deletes the 2nd column
+- resume user A's uploads
+
+The result of this is that user B will see the value in new 2nd column which used to be 3rd column.


What if you modify/delete the last row?

Should be the same as above. The patch will try to be applied to an object that doesn't exist. I didn't spell these out because these are all the same class of issue: an array of objects.

tealefristoe · 2026-03-05T03:23:22Z

docs/group-documents.md

+- resume User A's updates (with a delay)
+- User B starts editing a different cell, but doesn't hit enter or tab. This has to happen before User A's updates are applied to User B's document
+
+In this case User B's edits will be lost. This happens even though they are in a different cell. The cursor and focus of User B is lost when the table is updated to show User A's changes.


This is an issue we went through great lengths to avoid in the collaborative table codap plugin.

Yup it will be hard to get this right.

tealefristoe · 2026-03-05T16:36:32Z

src/models/history/firestore-history-manager-concurrent.ts

+      // If we made it here then the transaction succeeded so we can remove the entriesToUpload
+      // from the queue.
+      this.completedHistoryEntryQueue.splice(0, entriesToUpload.length);


If there are 2 entries to upload, and the first succeeds, but the second fails, we won't update completedHistoryEntryQueue and the first entry will stay in the queue. Then if we try to upload entries again, we'll upload the first one again. It seems like we want to update completeHistoryEntryQueue every time we upload an entry, not when a whole batch succeeds.

Because tis is running in a transaction, if any of the writes fail then they all will be rolled back. So it will be all or nothing. This is the reason to not update the completedHistoryEntryQueue inside of the transaction. There are supposed to be no side effects in the transaction because it can be run multiple times.

scytacki changed the base branch from master to CLUE-341-history-manager-refactor February 5, 2026 18:08

github-actions bot temporarily deployed to development February 5, 2026 18:11 Inactive

github-actions bot temporarily deployed to development February 5, 2026 18:39 Inactive

scytacki added 6 commits February 5, 2026 17:02

CLUE-341 CLUE-341 Add live group document update support

9562e2f

CLUE-341 fix initial load of group docs

2fda89d

CLUE-341 update doc with remaining work

015f90f

CLUE-341 renamed documentation

db09f6e

CLUE-341 document local history missing previous remote entries

140a497

CLUE-341 remove some wonky-ness in initial history filtering

5f71b20

scytacki force-pushed the CLUE-341-group-doc-live branch from 04d1f1d to 5f71b20 Compare February 5, 2026 22:03

github-actions bot temporarily deployed to development February 5, 2026 22:08 Inactive

scytacki requested a review from Copilot February 6, 2026 16:44

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Base automatically changed from CLUE-341-history-manager-refactor to group-documents February 6, 2026 17:11

scytacki mentioned this pull request Feb 6, 2026

WIP: CLUE-341-group-doc-live-update #2732

Closed

CLUE-341 address PR comments

760a97d

most complex change is better error handling in uploadQueuedHistoryEntries. The approach doesn't fully fix the problem, but at least it prevents a messed up uploadInProgress state.

scytacki marked this pull request as ready for review March 4, 2026 20:37

scytacki requested a review from tealefristoe March 4, 2026 22:11

tealefristoe approved these changes Mar 5, 2026

View reviewed changes

scytacki mentioned this pull request Mar 5, 2026

CLUE-341-group-docs-merge #2757

Merged

CLUE-341 address PR comment, and clean up old code comment

55d021b

scytacki merged commit 1eec6bc into group-documents Mar 6, 2026
6 of 7 checks passed

scytacki deleted the CLUE-341-group-doc-live branch March 6, 2026 03:04

	The result of this is that user B will see different object colors than user B.
	The result of this is that user A will see different object colors than user B.


		The result of this is that user B will see different object colors than user B.

		This behavior should happen for any common drawing object properties: fill color, stroke width, position, rotation, flipping. If there is a property like "text" of a text object that doesn't exist on other objects, this seems to just be ignored. It doesn't show an error in the console.

Conversation

scytacki commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cypress bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests for review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tealefristoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scytacki commented Feb 5, 2026 •

edited

Loading

codecov bot commented Feb 5, 2026 •

edited

Loading

cypress bot commented Feb 5, 2026 •

edited

Loading