CLUE-368 extracts Firestore history saving#2713
Conversation
collaborative-learning
|
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-368-extract-firestore-history-saving
|
| Run status |
|
| Run duration | 02m 59s |
| Commit |
|
| Committer | Scott Cytacki |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2713 +/- ##
==========================================
- Coverage 86.75% 86.74% -0.02%
==========================================
Files 809 810 +1
Lines 43316 43331 +15
Branches 11098 11097 -1
==========================================
+ Hits 37580 37587 +7
- Misses 5405 5413 +8
Partials 331 331
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extracts Firestore history-saving functionality from the TreeManager into a new FirestoreHistoryManager class, improving separation of concerns and enabling different document types to implement custom history-saving strategies.
Changes:
- Created new
FirestoreHistoryManagerclass to handle Firestore-specific history saving - Replaced direct Firestore references in
TreeManagerwith a listener pattern usingHistoryEntryCompletedListener - Updated document initialization to instantiate and register the Firestore history manager
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/models/history/history-firestore.ts | Added FirestoreHistoryManager class with methods moved from TreeManager |
| src/models/history/tree-manager.ts | Removed Firestore-specific code and added listener pattern for history entry completion |
| src/models/stores/documents.ts | Instantiated FirestoreHistoryManager and registered it as a listener |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kswenson you might be interested in this one too. It is moving towards a better separation of the firestore history syncing from the core history system. This ought to reduce the number of changes CODAP has to make to the tree-manager.ts file. |
We'd love to adopt this and any other CLUE improvements into CODAP, but probably in the 3.1 time frame at the earliest. |
tealefristoe
left a comment
There was a problem hiding this comment.
Looks good 👍
This was a good opportunity for me to get a glimpse into this part of the codebase. I noticed that there's now an import cycle, but I don't know if that's a problem or not.
| import { UserContextProvider } from "../stores/user-context-provider"; | ||
| import { HistoryEntry, HistoryEntrySnapshot } from "./history"; | ||
| import { TreeAPI } from "./tree-api"; | ||
| import { CDocumentType } from "./tree-manager"; |
There was a problem hiding this comment.
This creates an import cycle.
There was a problem hiding this comment.
Good point, I refactored it to take care of this.
CLUE-368
This extracts the Firestore history saving from the CLUE history system.
There are a few reasons to do this: