feat: Expose hasStagedChanges/hasStagedChangesChanged on IContainerRuntimeBase by dannimad · Pull Request #27648 · microsoft/FluidFramework

dannimad · 2026-07-07T17:46:49Z

Description

IContainerRuntimeBase now exposes hasStagedChanges, a boolean indicating whether there are any changes submitted while in Staging Mode (via enterStagingMode) that have not yet been discarded or committed. A new hasStagedChangesChanged event is emitted whenever this value changes.

This is distinct from isDirty/dirty: a container runtime can be dirty due to ordinary unacknowledged local changes without having any staged changes, and vice versa. Per review feedback, StagingMode-related concepts stay on IContainerRuntimeBase for now rather than being exposed on IContainer.

containerRuntime.on("hasStagedChangesChanged", (hasStagedChanges) => {
        // update UI to reflect whether there are staged changes awaiting commit/discard
});

const hasStagedChanges = containerRuntime.hasStagedChanges;

Test coverage mirrors the existing isDirty/dirty test suites:

PendingStateManager unit tests for hasStagedChanges()
ContainerRuntime baseline flag tests across Attached/Attaching/Detached states
ContainerRuntime staging-mode lifecycle tests (commitChanges/discardChanges)
ContainerRuntime hasStagedChangesChanged event tests
End-to-end lifecycle tests in local-server-tests (documentStagedChanges.spec.ts)

While adding tests, found and fixed a real bug: hasStagedChanges/hasStagedChangesChanged were not refreshed after flush() alone, so the cached value could remain stale (false) even with staged ops pending in the PendingStateManager. Fixed by calling updateHasStagedChangesState() at the end of ContainerRuntime.flush().

Reviewer Guidance

The review process is outlined on this wiki page.

The flush() fix is the only behavioral change outside of tests — please double check the reasoning around staleness in updateHasStagedChangesState().

…gesChanged Adds robust test coverage for the hasStagedChanges/hasStagedChangesChanged feature, mirroring the existing isDirty/dirty test patterns: - PendingStateManager unit tests for hasStagedChanges() - ContainerRuntime baseline "Staged changes flag" tests across Attached/Attaching/Detached states - ContainerRuntime staging-mode lifecycle tests (commitChanges/discardChanges) - ContainerRuntime hasStagedChangesChanged event tests - End-to-end Document Staged Changes lifecycle tests in local-server-tests Also fixes a real bug found while testing: hasStagedChanges/ hasStagedChangesChanged were not refreshed after flush() alone, so the cached value could remain stale even with staged ops pending in the PendingStateManager. Now updateHasStagedChangesState() is called at the end of ContainerRuntime.flush(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-07T17:47:52Z

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (1077 lines, 17 files), I've queued these reviewers:

Correctness — logic errors, race conditions, lifecycle issues
Security — vulnerabilities, secret exposure, injection
API Compatibility — breaking changes, release tags, type design
Performance — algorithmic regressions, memory leaks
Testing — coverage gaps, hollow tests

How this works

Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.
Tick Start review below to dispatch the review fleet.
After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.
This comment updates as new commits land; your reviewer selections are preserved.
Start review

Copilot

Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.

Renames the hasStagedChanges changeset from a manually-chosen descriptive filename to a randomly-generated one produced by `flub changeset add --empty`, matching the normal changeset creation flow used elsewhere in the repo. Content is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…imeBase Per review feedback, StagingMode-related concepts should stay on IContainerRuntimeBase for now rather than being exposed on IContainer directly. This commit: - Removes hasStagedChanges/hasStagedChangesChanged from IContainer, IContainerEvents, and IContainerContext.updateStagedChangesState (container-definitions, container-loader). - Moves hasStagedChanges/hasStagedChangesChanged from IContainerRuntime (container-runtime-definitions) to IContainerRuntimeBase (runtime-definitions), alongside the existing enterStagingMode/ inStagingMode/stagingModeChanged surface. - Updates ContainerRuntime's wiring to no longer notify the container layer of staged-changes state (only dirty state remains plumbed through to the container). - Updates existing tests and mocks accordingly, and rewrites documentStagedChanges.spec.ts to assert only at the ContainerRuntime level. - Regenerates API reports and type tests for the affected packages, and updates the changeset to reflect the new API surface location. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

markfields

Looks pretty good, but I think there's a mismatch with what expected behavior probably is for a consumer - they'll expect hasStagedChanges to be true as soon as the change happens, regardless of when the batch flushes from the Outbox.

markfields · 2026-07-08T21:58:45Z

+			}
+		}
+
+		this.updateHasStagedChangesState();


This feels excessive — updateDocumentDirtyState is called from ~6 places (process, resubmit, connect, rollback, etc.) and most of those can't change staged-changes state. The only transition this catches that the other two call sites (flush() line 3728, commitChanges line 3922) don't is discardChanges (line 3910 calls updateDocumentDirtyState but not updateHasStagedChangesState).

Consider dropping this from updateDocumentDirtyState and adding an explicit this.updateHasStagedChangesState() call in the discardChanges lambda (after the updateDocumentDirtyState() at line 3910). More surgical, avoids running the check on every unrelated dirty-state update.

Agreed — confirmed 6 call sites, only discardChanges/commitChanges/flush/submitMessage can actually affect staged state. Removed the blanket call from updateDocumentDirtyState and added explicit this.updateHasStagedChangesState() calls only where staged state can truly change: discardChanges, and submitMessage (new — see next item).

markfields · 2026-07-08T21:58:45Z

+					// orderSequentially with EnableRollback calls enterStagingModeCore(silent=true) internally,
+					// which suppresses stagingModeChanged. hasStagedChangesChanged is driven by
+					// updateDocumentDirtyState/updateHasStagedChangesState instead, which are not gated by
+					// the `silent` flag, so it CAN fire in this scenario.


Why is this the preferred design? Or are we ambivalent and just documenting what it does (which is the simplest approach)

Just documenting. I think this could be a scenario where we could ask clarification and fix in a follow up

markfields · 2026-07-08T21:58:45Z

+			// Use a low threshold so staged ops are auto-flushed to the PendingStateManager (which is what
+			// hasStagedChanges checks) right away instead of waiting for the default (1000 ops) batch size.
+			runtimeOptions: { stagingModeAutoFlushThreshold: 1 },


Ok this raises an interesting question. I had forgotten about this autoflush feature. I wonder if hasStagedChanges should also return true if the outbox is nonempty and we're in staging mode. I think the difference between outbox and PSM is irrelevant to a consumer using Staging Mode.

This was a real bug: while in Staging Mode, a submitted op sits in the Outbox until the next flush, and during that window hasStagedChanges incorrectly reported false. Fixed computeCurrentHasStagedChanges() to also check this.inStagingMode && this.outbox.containsUserChanges(). Updated submitMessage to call updateHasStagedChangesState() explicitly, and updated/added tests in both containerRuntime.spec.ts and documentStagedChanges.spec.ts to assert the value is true immediately on submit, before flush.

markfields · 2026-07-08T21:58:45Z

 				});
 			});
+
+			describe("hasStagedChangesChanged event", () => {


Consider adding a unit test here that enters staging mode, submits no ops, then exits (both commit and discard paths) — asserting hasStagedChangesChanged is never emitted (event count stays 0) and hasStagedChanges remains false throughout. The e2e test checks the property at line 171 but doesn't assert the event isn't fired, and this is the right place for that kind of negative assertion as a unit test.

Missing test coverage (3 separate suggestions)
Added all three:

Unit tests for entering/discarding and entering/committing staging mode with no ops — assert hasStagedChangesChanged never fires and hasStagedChanges stays false.
documentStagedChanges.spec.ts: new test asserting the commit path — isDirty stays true (in-flight/unacked) right after commitChanges() while hasStagedChanges flips to false.
documentStagedChanges.spec.ts: new test loading a second, freshly-loaded container from the same document while the first has uncommitted staged changes — asserts the fresh container reports hasStagedChanges: false (added a loadContainer helper using loadExistingContainer for this).

github-actions · 2026-07-10T06:22:49Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-website@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-website@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  294296 links
    1937 destination URLs
    2187 URLs ignored
       0 warnings
       0 errors

github-actions · 2026-07-10T06:54:43Z

Bundle size comparison

Base commit: 006b9fcdfe1a7083da88f7b2faf010ec33cad26c
Head commit: 45fd1f946058525a41a19d17b63ea2ce79a1f95f

Notable changes

🔴 azureClient.js: parsed 619098 → 619939 (+841), gzip 164826 → 164984 (+158)
🔴 odspClient.js: parsed 591824 → 592665 (+841), gzip 158912 → 159096 (+184)
🔴 aqueduct.js: parsed 525587 → 526407 (+820), gzip 140710 → 140850 (+140)
🔴 containerRuntime.js: parsed 303937 → 304736 (+799), gzip 83213 → 83322 (+109)

Per-bundle deltas

`@fluid-example/bundle-size-tests`

🔴 azureClient.js: parsed 619098 → 619939 (+841), gzip 164826 → 164984 (+158)
🔴 odspClient.js: parsed 591824 → 592665 (+841), gzip 158912 → 159096 (+184)
🔴 aqueduct.js: parsed 525587 → 526407 (+820), gzip 140710 → 140850 (+140)
fluidFramework.js: parsed 392708 → 392828 (+120), gzip 111481 → 111542 (+61)
sharedTree.js: parsed 382095 → 382208 (+113), gzip 108869 → 108924 (+55)
🔴 containerRuntime.js: parsed 303937 → 304736 (+799), gzip 83213 → 83322 (+109)
sharedString.js: parsed 175984 → 175991 (+7), gzip 49445 → 49453 (+8)
experimentalSharedTree.js: parsed 160798 → 160798 (0), gzip 45804 → 45804 (0)
matrix.js: parsed 159845 → 159852 (+7), gzip 45411 → 45418 (+7)
loader.js: parsed 145256 → 145270 (+14), gzip 39063 → 39077 (+14)
odspDriver.js: parsed 104329 → 104350 (+21), gzip 32646 → 32635 (-11)
directory.js: parsed 66616 → 66623 (+7), gzip 18532 → 18541 (+9)
748.js: parsed 58793 → 58793 (0), gzip 17827 → 17827 (0)
map.js: parsed 46709 → 46716 (+7), gzip 14310 → 14318 (+8)
odspPrefetchSnapshot.js: parsed 45642 → 45656 (+14), gzip 15277 → 15285 (+8)
985.js: parsed 44491 → 44491 (0), gzip 13726 → 13726 (0)
summarizerDelayLoadedModule.js: parsed 30749 → 30749 (0), gzip 7753 → 7753 (0)
socketModule.js: parsed 26476 → 26483 (+7), gzip 7885 → 7895 (+10)
createNewModule.js: parsed 12480 → 12480 (0), gzip 4786 → 4786 (0)
summaryModule.js: parsed 3797 → 3797 (0), gzip 1860 → 1860 (0)
connectionState.js: parsed 724 → 724 (0), gzip 429 → 429 (0)
sharedTreeAttributes.js: parsed 666 → 673 (+7), gzip 432 → 442 (+10)
debugAssert.js: parsed 429 → 429 (0), gzip 299 → 299 (0)
FluidFramework-HashFallback.js: parsed 422 → 422 (0), gzip 316 → 316 (0)

markfields · 2026-07-10T18:55:12Z

+	 * (see {@link @fluidframework/runtime-definitions#IContainerRuntimeBaseInternal.enterStagingMode})
+	 * that have not yet been discarded or committed.
+	 *
+	 * @remarks This is distinct from {@link ContainerRuntime.isDirty}: a container may be dirty due to


The "and vice versa" should be kept here after all! Or explain the case specifically - The new unit test (hasStagedChanges is true for a non-dirtyable op sitting in the Outbox while staging) explicitly demonstrates that hasStagedChanges can be true while isDirty is false — that's the "vice versa" case.

markfields · 2026-07-10T18:55:12Z

+	 * not yet been discarded or committed.
+	 *
+	 * @remarks This is distinct from {@link @fluidframework/container-runtime-definitions#IContainerRuntime.isDirty}: a container may be dirty due to
+	 * ordinary unacknowledged local changes without having any staged changes.


Same here — "and vice versa" should be kept. A non-dirtyable op submitted during staging mode results in hasStagedChanges=true but isDirty=false (as the new unit test demonstrates).

markfields · 2026-07-10T18:55:12Z

+
+					containerRuntime.on("hasStagedChangesChanged", (hasStagedChanges) => {
+						events.push(hasStagedChanges);
+						hasStagedChangesAtEventTime.push(containerRuntime.hasStagedChanges);


nit: just assert here that containerRuntime.hasStagedChanges === hasStagedChanges? And remove hasStagedChangesAtEventTime

markfields · 2026-07-10T18:55:12Z

+			);
+		});
+
+		it("returns true if a staged message is an empty batch (runtimeOp undefined)", () => {


Side note - I don't think this is possible anyway.

markfields · 2026-07-10T18:55:12Z

 		const dirty: boolean = this.computeCurrentDirtyState();

-		if (this.lastEmittedDirty === dirty) {
+		if (this.lastEmittedDirty !== dirty) {


Why is this being inverted? Especially considering that updateHasStagedChangesState follows the other form!

It's a nit, but I'd revert this to reduce the diff and keep them symmetrical.

markfields · 2026-07-10T18:55:12Z

+
+		stageControls.commitChanges();
+
+		checkStagedChangesState("after commitChanges", false, 1, 1);


Also assert isDirty is true here then false after ensureSynchronized?

[edit] - Oh, there is that test below. Are they redundant otherwise?

CraigMacomber

The changeset seems fine from a docs perspective.

dannimad and others added 2 commits July 6, 2026 20:23

hasStagedChanges

1a70501

Copilot AI review requested due to automatic review settings July 7, 2026 17:46

dannimad requested review from a team as code owners July 7, 2026 17:46

Copilot started reviewing on behalf of dannimad July 7, 2026 17:47 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

github-actions Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread .changeset/five-icons-build.md Outdated

github-actions Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread .changeset/five-icons-build.md Outdated

dannimad changed the title ~~feat: Expose hasStagedChanges/hasStagedChangesChanged on Container and IContainerRuntime~~ feat: Expose hasStagedChanges/hasStagedChangesChanged on IContainerRuntimeBase Jul 8, 2026

markfields reviewed Jul 8, 2026

View reviewed changes

dannimad added 5 commits July 9, 2026 15:33

comments

71b836f

Merge remote-tracking branch 'origin/main' into hasStagedChanges

e6ece7e

tsc

018cd16

tsc

12e29dd

check outbox size

45fd1f9

markfields approved these changes Jul 10, 2026

View reviewed changes

dannimad enabled auto-merge (squash) July 10, 2026 20:01

CraigMacomber approved these changes Jul 10, 2026

View reviewed changes


		stageControls.commitChanges();

		checkStagedChangesState("after commitChanges", false, 1, 1);

Uh oh!

Conversation

dannimad commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Guidance

Uh oh!

github-actions Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markfields 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 10, 2026

linkcheck output

Uh oh!

github-actions Bot commented Jul 10, 2026

Bundle size comparison

Notable changes

@fluid-example/bundle-size-tests

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!

CraigMacomber left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dannimad commented Jul 7, 2026 •

edited

Loading

github-actions Bot commented Jul 7, 2026 •

edited

Loading

`@fluid-example/bundle-size-tests`