fix: do not duplicate task context files in memory #15776
Conversation
|
@AlexandraBuzila could you please have a look, TIA |
|
@xai LGTM and works as expected, thanks! 🎉 |
|
The idea behind using the in-memory store even when the file store was active was to avoid having to reread the disk every time a summary is activated. Would it be possible to identify the mechanism that is causing the apparent duplication - presumably use of different ID's for the same summary? - and ensure that that doesn't occur, but we still get the advantages of keeping the summaries in memory? The code in lines 128-131 is intended to prevent duplication. Or @JonasHelming, are we ok with reading the disk for every request, in which case, we likely just want to remove the in-memory store completely when the disk-based store is active? The main downside there is that we won't have on hand a list of things for which we know summaries - we'd have to check before populating the quick pick, etc. |
| @@ -142,7 +142,6 @@ export class TaskContextFileStorageService implements TaskContextStorageService | |||
| summary.uri = uri; | |||
| await this.fileService.writeFile(uri, BinaryBuffer.fromString(content)); | |||
| } | |||
| this.inMemoryStorage.store(summary); | |||
| this.onDidChangeEmitter.fire(); | |||
There was a problem hiding this comment.
If we do remove this store call, then we should remove the emitter firing - it will fire when the file watchers pick up this summary.
ACK, this requires some debugging then! |
Fixes eclipsesource/theia/257 Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
|
@colin-grant-work Just pushed another try by extending the existing deduplication logic instead of removing the store(). |
Fixes eclipsesource#257
What it does
Removes a redundant store() of new task summary files to the in-memory storage.
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers