Skip to content

CLUE-341 Refactor FirestoreHistoryManager and TreeManager#2750

Merged
scytacki merged 4 commits intogroup-documentsfrom
CLUE-341-history-manager-refactor
Feb 6, 2026
Merged

CLUE-341 Refactor FirestoreHistoryManager and TreeManager#2750
scytacki merged 4 commits intogroup-documentsfrom
CLUE-341-history-manager-refactor

Conversation

@scytacki
Copy link
Member

@scytacki scytacki commented Feb 5, 2026

CLUE-341

A refactoring before implementing live group document updates.

  • Move more methods from TreeManager into FirestoreHistoryManager.
  • Update the FirestoreHistoryManager to make the method names more clear.
  • Update the FirestoreHistoryManager so the concurrent subclass can add its new features without duplicating code
  • Remove more complex onHistoryEntryCompletedListener system, and just have the TreeManager have a single HistoryManager instance that it calls onHistoryCompleted on. This HistoryManager type used by TreeManager is just an simple interface so there isn't a circular dependency with FirestoreHistoryManager.

@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 74.59459% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.78%. Comparing base (edf10b5) to head (e8382ef).
⚠️ Report is 7 commits behind head on group-documents.

Files with missing lines Patch % Lines
...ls/history/firestore-history-manager-concurrent.ts 13.79% 25 Missing ⚠️
src/models/history/firestore-history-manager.ts 85.95% 17 Missing ⚠️
src/models/stores/documents.ts 62.50% 3 Missing ⚠️
src/models/history/tree-manager.ts 75.00% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           group-documents    #2750      +/-   ##
===================================================
+ Coverage            85.76%   85.78%   +0.01%     
===================================================
  Files                  819      820       +1     
  Lines                44002    44039      +37     
  Branches             11249    11255       +6     
===================================================
+ Hits                 37739    37778      +39     
+ Misses                6247     6245       -2     
  Partials                16       16              
Flag Coverage Δ
cypress-regression 76.73% <70.12%> (+0.03%) ⬆️
cypress-smoke 43.85% <33.53%> (+0.02%) ⬆️
jest 49.51% <49.18%> (+0.08%) ⬆️

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
Copy link

cypress bot commented Feb 5, 2026

collaborative-learning    Run #17643

Run Properties:  status check passed Passed #17643  •  git commit e8382efee4: CLUE-341 address PR comments
Project collaborative-learning
Branch Review CLUE-341-history-manager-refactor
Run status status check passed Passed #17643
Run duration 03m 00s
Commit git commit e8382efee4: CLUE-341 address PR comments
Committer Scott Cytacki
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the history management system by consolidating functionality into FirestoreHistoryManager and simplifying the interaction between TreeManager and history managers. The refactoring prepares the codebase for implementing live group document updates.

Changes:

  • Moved history-related methods and state from TreeManager to FirestoreHistoryManager
  • Replaced the listener-based system with a single IHistoryManager interface
  • Split FirestoreHistoryManager into base and concurrent subclasses to avoid code duplication

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/models/stores/documents.ts Updated to use new IFirestoreHistoryManagerArgs interface and set history manager on tree manager
src/models/history/tree-manager.ts Removed history loading logic and replaced listener system with single history manager reference
src/models/history/tree-manager-history.test.ts Removed tests for mirrorHistoryFromFirestore (moved to history manager tests)
src/models/history/history-firestore.ts Added IFirestoreHistoryEntryDoc interface to return structured history documents
src/models/history/firestore-history-manager.ts Added history status tracking, remote history syncing, and consolidated history management logic
src/models/history/firestore-history-manager.test.ts New test file covering history loading and status updates for the history manager
src/models/history/firestore-history-manager-concurrent.ts New file containing concurrent version for multi-user editing
src/components/playback/playback.tsx Updated to receive history manager as prop and use its status methods
src/components/document/history-view-panel.tsx Updated to handle new IFirestoreHistoryEntryDoc structure
src/components/document/canvas.tsx Creates history manager instance for playback documents

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

@scytacki scytacki marked this pull request as ready for review February 5, 2026 14:06
Copy link
Contributor

@tealefristoe tealefristoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments, but looks good 👍

Base automatically changed from CLUE-341-upgrade-jest to group-documents February 6, 2026 17:10
@scytacki scytacki merged commit 288fcec into group-documents Feb 6, 2026
23 of 27 checks passed
@scytacki scytacki deleted the CLUE-341-history-manager-refactor branch February 6, 2026 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants