Skip to content

Conversation

@dannimad
Copy link
Contributor

@dannimad dannimad commented Dec 30, 2025

This PR refactors snapshot refresh functionality by extracting it from SerializedStateManager into a new dedicated SnapshotRefresher class, improving separation of concerns,maintainability and testing. Current e2e snapshot refresh tests are disabled due to flakiness so this PR adds back some test coverage for those scenarios.

Copilot AI review requested due to automatic review settings December 30, 2025 18:44
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 snapshot refresh functionality by extracting it from SerializedStateManager into a new dedicated SnapshotRefresher class, improving separation of concerns and maintainability. The PR includes comprehensive unit tests for the new class.

Key changes:

  • Extracted SnapshotRefresher class to manage periodic snapshot refresh operations with configurable timing
  • Added comprehensive unit test coverage (793 lines) for SnapshotRefresher including edge cases, disposal, timer management, and error handling
  • Refactored SerializedStateManager to delegate snapshot refresh responsibilities to SnapshotRefresher

Reviewed changes

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

File Description
packages/loader/container-loader/src/snapshotRefresher.ts New class that encapsulates snapshot refresh logic including timer management, promise tracking, and refresh orchestration
packages/loader/container-loader/src/test/snapshotRefresher.spec.ts Comprehensive unit tests for SnapshotRefresher covering constructor, timer management, refresh flows, disposal, and group ID snapshots
packages/loader/container-loader/src/serializedStateManager.ts Refactored to use SnapshotRefresher for snapshot refresh operations, removing duplicate code and delegating refresh responsibilities

@anthony-murphy anthony-murphy removed their assignment Jan 5, 2026
@anthony-murphy
Copy link
Contributor

@dannimad please do not assign PRs to me. it breaks my notification settings

this.snapshotInfo = this.latestSnapshot;
this.latestSnapshot = undefined;
this.refreshTimer?.restart();
this.snapshotRefresher?.clearLatestSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

these don't seem equivalent, and its not clear why it is being changed. generally, we should avoid mixing refactoring with behavior changes, as it makes it hard to review and detect un-intended behavior changes

Copy link
Contributor Author

@dannimad dannimad Jan 6, 2026

Choose a reason for hiding this comment

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

clearLatestSnapshot is the equivalent of this.latestSnapshot = undefined, we're just clearing snapshotRefresher copy as well. I remove refreshTimer.restart() given that we will restart that timer right after onSnapshotRefreshed inside the refresher. I agree it is not clear to see but this is not a behavior change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants