fix: preserve BubbleBench container compatibility - #27926
Conversation
…API item kind (#27904) Makes API links shorter / easier to write in the standard case. Also adds unit tests, including vitest infra for unit testing the site's React components.
## Description The existing review skill was often counterproductive for me as it was very specific about what to review against, often worse than what the model would come up with on its own. These changes correct its assumptions, making the skill much more generally useful, even with less powerful models, explicitly directing it to handle removes with different names (like my upstream), compare to the merge point with the target branch not its head, handle local changes (untracked, modifications and staged changes) etc. The new approach checks for an existing PRs as a heuristic to find the correct upstream branch and remote, then looks for an upstream for the https://github.com/microsoft/FluidFramework repo, regardless of its name. It also warns if the merge base and target branch are too out of sync by commit count, not just diff size (which was slow and confusing when I hit it in my fork due to using the wrong remote). I also refactored our two existing skills which hard coded origin/main to both use this extracted common logic for improved consistency and maintainability. I used this skill to review itself, with a couple models and at a couple points in the process (like when it was just local changes on main, when it was staged on a branch, and after the PR was created. Even Haiku 4.5 could follow the steps and produce a review using this skill (its review quality wasn't great, but it found the correct diff to review).
…27573) ## Description Adds a new `@fluid-example/claims-example` app that demonstrates the **Claims DDS** (`@fluid-internal/claims`) running inside a real Fluid container, and makes a small supporting change to the Claims DDS API itself. The Claims DDS is an internal building block for first-writer-wins ownership: clients compete to bind a key to a value, and the ordering service picks a single winner that every client converges on. This example wires it up by hand inside a custom `ClaimsDataObject`, since the Claims DDS has no public consumption path yet. **What the example shows** - A `ClaimsDataObject` owns a single Claims DDS (stored by handle on the root) and exposes a narrow `trySetClaim` / `getOwner` surface, so the view never touches the Claims DDS directly. - Each browser tab gets its own `claimant` identity. Opening the same container URL in a second tab gives you a competing client. - There is a small, fixed set of known keys (`ClaimKey1`, `ClaimKey2`). Claiming a key creates a fresh `SharedDirectory` that records the owner and binds its handle as the claim value, so every client resolves the winning handle to the same shared object — no key enumeration or side structure required. - **First-writer-wins:** the first client to claim a key wins, and a losing client is switched to reflect the winner. Connected claims come back `Pending` and settle after the op roundtrip; detached claims resolve synchronously. - The view reads each key's owner live from the resolved backing directory and re-renders on the Claims DDS `claimed` event, so ownership stays in sync both locally and remotely. The app runs against the shared `@fluid-example/example-driver`, so the backing service (local/t9s, ODSP, …) is selected at build time rather than hard-coded. **Claims DDS change** Removes `currentValue` from the Claims DDS result types (`ClaimConfirmation` / `ClaimResult`), which also makes them non-generic. Callers that need the current owner read it back via `claims.get(key)` — which is fresher and, for write-once keys, immutable. This narrows the `@internal` API surface with no change to claim/consensus behavior. ## Reviewer Guidance The review process is outlined on [this wiki page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). The example deliberately uses `/internal` entry points because the Claims DDS has no public consumption path, and we don't intend to add one. [AB#74216](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/74216) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e4794d0-196d-45b7-b9f4-1cc94acd1b45
## Description Prevent the network-isolated build-tools and client pipelines from contacting `registry.npmjs.org`. The shared pnpm setup continues to create and authenticate a temporary user-level `.npmrc` for the configured Azure Artifacts registry. It now: - passes that userconfig explicitly while installing pnpm; - fails fast unless both npm and pnpm resolve the expected registry; - publishes the existing CI mirror workarounds (`trustPolicy=off` and `minimumReleaseAge=0`) as job variables; and - sets `trustLockfile=true` for subsequent CI installs so pnpm 11 does not query registry metadata to reapply those policies to every dependency already represented by a committed lockfile. `trustLockfile` does not affect lockfile creation or updates, frozen-lockfile consistency, structural validation, or downloaded tarball integrity. The setting is emitted only by the ADO template, so local dependency-update workflows retain their existing behavior. Shared build, lint, and test scripts now run through `Bash@3` instead of `Npm@1`. `Npm@1` replaces the user-level npm configuration, which allowed nested registry operations such as compatibility-test `pnpm view` calls to fall back to npmjs. Bash preserves the authenticated job-level userconfig. The npmrc setup documentation now calls out this behavior for scripts that directly or indirectly perform registry operations. ## Reviewer Guidance The review process is outlined on [this wiki page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). Please focus on: - registry verification in `include-install-pnpm.yml`; - the CI-only `trustLockfile` tradeoff described above; and - the intentional use of Bash for build and test scripts that may perform nested registry operations. ### Validation Build Tools and Client Packages pipelines all show CFS Client COMPLIANT ✅ Prior validation done with forced cold pnpm cache, will be done again after review signoff, before merging: - Internal cold-cache Build - build-tools run 415527: cache miss and CFSClean compliant. - Internal cold-cache Build - client packages run 415518: cache miss and CFSClean compliant. The unrelated RealsvcTinyliciousTest job failed; Build and Coverage tests succeeded. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Abram Sanderson <Abram.Sanderson@microsoft.com>
[How contribute to this repo](https://github.com/microsoft/FluidFramework/blob/main/CONTRIBUTING.md). [Guidelines for Pull Requests](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). ## Description The client packages pipeline failed in its telemetry upload stages after registry validation was added by [PR #27893](#27893). The `ado-feeds-ff-download-only` value contained trailing whitespace, so the validation logic normalized it to `…/registry/ /` while npm correctly resolved `…/registry/`, causing an otherwise successful build to fail. Trim surrounding whitespace before normalizing the configured registry to one trailing slash. This preserves the strict registry safety check while accepting equivalent registry values with incidental whitespace. Validated the normalization for registry values with trailing whitespace, surrounding whitespace, and an existing trailing slash. ## Reviewer Guidance The review process is outlined on [this wiki page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). Please verify that trimming whitespace is appropriate before comparing npm and pnpm's resolved registries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Description Adds tests that assert the ordering guarantees of SharedTree's change events (`nodeChanged`, `treeChanged`, `rootChanged`) and their interaction with `withBufferedTreeEvents`. These tests document and lock in the following behaviors: - `nodeChanged` fires before `treeChanged` on the same node - Events propagate bottom-up through the tree hierarchy - `rootChanged` fires after `nodeChanged`/`treeChanged` (via `afterBatch`) - `withBufferedTreeEvents` inverts `rootChanged` relative to buffered node events - Node events respect ordering within buffered flushes Also updates a TODO comment in `treeChangeEvents.ts` to document the confirmed bottom-up ordering behavior. ## Reviewer Guidance [Reviewer Guidance Wiki](https://dev.azure.com/fluidframework/internal/_wiki/wikis/FF%20Internal%20Wiki/1217/Reviewer-guidance) The test file is self-contained. The only production code change is replacing a TODO with documentation (line 69 of `treeChangeEvents.ts`). --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use Fluid-owned iterable contracts and a minimal read-only bubble collection so both SharedTree arrays and native arrays satisfy BubbleBench's shared model types. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
00800f4 to
aea1aae
Compare
|
Superseded by a main-targeting PR because GitHub does not allow changing the base of a pull request already registered in a native stack. |
|
Superseded by #27928, which targets main independently. |
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (29 lines, 5 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
This PR broadens several areas of the repo: it updates the Fluid Framework website’s API short-linking to resolve versioned API doc paths (with new Vitest unit tests), adjusts BubbleBench’s collection typings to be compatible across Tree and native array implementations, updates the Claims DDS result shapes (removing currentValue) and adds a new claims-example app, adds Tree change-event ordering tests/documentation, and updates Azure Pipelines templates to prefer Bash over Npm@ tasks.
Changes:
- Website: make
ApiLinkresolve the correct versioned doc path from Docusaurus’ active version, and add Vitest-based unit tests + scripts. - BubbleBench: introduce a minimal “readonly array-ish + iterable” contract using
FluidIterable. - Infra & examples: shift pipeline templates from
Npm@toBash@, update Claims DDS result types, and add a new claims example app + Tree event ordering tests.
Reviewed changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/vitest.config.mts | Adds Vitest configuration for website unit tests. |
| website/test/unit/shortLinks.test.ts | Adds unit coverage for ApiLink resolution and error cases. |
| website/test/tsconfig.json | Adjusts test TS config for ESM/bundler-style resolution. |
| website/src/components/shortLinks.tsx | Updates ApiLink to resolve versioned doc paths via active Docusaurus docs list. |
| website/README.md | Documents unit vs site test suites and related commands. |
| website/package.json | Adds vitest + splits test scripts into unit/site/aggregate. |
| website/playwright.config.ts | Scopes Playwright to site tests only. |
| website/pnpm-lock.yaml | Updates lockfile for Vitest/Vite-related dependencies. |
| website/docs/testing/telemetry.mdx | Updates ApiLink usage to omit apiType. |
| website/versioned_docs/version-1/testing/telemetry.mdx | Same ApiLink usage update for versioned docs. |
| website/versioned_docs/version-1/release-notes.mdx | Updates ApiLink usage to omit apiType. |
| website/versioned_docs/version-1/build/containers.mdx | Updates ApiLink usage to omit apiType. |
| website/versioned_docs/version-1/build/container-states-events.mdx | Updates ApiLink usage to omit apiType. |
| website/docs/start/tree-start.mdx | Updates ApiLink usage to omit apiType. |
| website/docs/build/experimental-features.mdx | Updates ApiLink usage to omit apiType. |
| website/docs/build/containers.mdx | Updates ApiLink usage to omit apiType. |
| website/docs/build/container-states-events.mdx | Updates ApiLink usage to omit apiType. |
| tools/pipelines/templates/include-test-task.yml | Switches test execution from Npm@1 to Bash@3. |
| tools/pipelines/templates/include-build-lint.yml | Switches build/lint execution from Npm@1 to Bash@3. |
| tools/pipelines/templates/include-setup-npmrc-for-download.yml | Clarifies guidance to avoid Npm@ tasks for registry-involved scripts. |
| tools/pipelines/templates/include-install-pnpm.yml | Adds registry validation + sets PNPM_CONFIG_TRUST_LOCKFILE in CI. |
| pnpm-lock.yaml | Adds workspace lock entries for the new claims example app. |
| PACKAGES.md | Adds @fluid-example/claims-example to the private examples list. |
| packages/dds/tree/src/simple-tree/api/treeChangeEvents.ts | Documents intended nodeChanged/treeChanged ordering semantics. |
| packages/dds/tree/src/test/simple-tree/api/treeChangeEventOrdering.spec.ts | Adds comprehensive test coverage for Tree change event ordering. |
| packages/dds/claims/src/interfaces.ts | Changes Claim result/confirmation types (removes currentValue, drops generics). |
| packages/dds/claims/src/claims.ts | Updates implementation to match new result/confirmation shapes. |
| packages/dds/claims/src/claimsFactory.ts | Updates usage snippet to instruct reading values via claims.get(...). |
| packages/dds/claims/src/test/claims.spec.ts | Updates tests to validate values via claims.get(...) instead of result payloads. |
| packages/dds/claims/README.md | Updates documentation to match new result/confirmation shapes. |
| examples/benchmarks/bubblebench/common/src/types.ts | Introduces FluidIterable-based minimal read-only collection contracts. |
| examples/benchmarks/bubblebench/ot/src/state.ts | Updates BubbleBench OT adapter to expose FluidIterable-compatible clients. |
| examples/benchmarks/bubblebench/experimental-tree/src/state.ts | Updates BubbleBench experimental-tree adapter to expose FluidIterable clients. |
| examples/benchmarks/bubblebench/shared-tree/src/appState.ts | Updates BubbleBench shared-tree adapter to expose FluidIterable clients. |
| examples/apps/claims-example/package.json | Adds new private example app package definition. |
| examples/apps/claims-example/README.md | Documents the claims example scenario and behavior. |
| examples/apps/claims-example/LICENSE | Adds MIT license file for the example package. |
| examples/apps/claims-example/webpack.config.cjs | Adds webpack build/dev-server config for the new example. |
| examples/apps/claims-example/tsconfig.json | Adds TS config for building/typechecking the example. |
| examples/apps/claims-example/eslint.config.mts | Adds eslint flat config wiring for the example. |
| examples/apps/claims-example/.gitignore | Adds ignores for build outputs and dependencies. |
| examples/apps/claims-example/.npmignore | Adds npm publish ignores (even though package is private). |
| examples/apps/claims-example/jest.config.cjs | Adds Jest+Puppeteer test configuration for the example. |
| examples/apps/claims-example/jest-puppeteer.config.cjs | Adds Puppeteer launch + dev-server wiring for tests. |
| examples/apps/claims-example/tests/claimsExample.test.ts | Adds browser tests validating claim propagation and race behavior. |
| examples/apps/claims-example/src/app.ts | Adds example app bootstrap + container creation/loading. |
| examples/apps/claims-example/src/view.tsx | Adds minimal React UI for claiming keys and showing owners. |
| examples/apps/claims-example/src/container/index.ts | Exposes entry-point types and runtime factory for the example. |
| examples/apps/claims-example/src/container/runtimeFactory.ts | Implements runtime factory exposing a default Claims data store entry point. |
| examples/apps/claims-example/src/container/claimsDataObject/interface.ts | Defines the view-facing data object interface/events. |
| examples/apps/claims-example/src/container/claimsDataObject/index.ts | Exports data object factory and types. |
| examples/apps/claims-example/src/container/claimsDataObject/claimsDataObject.ts | Implements a DataObject wrapping the Claims DDS with a simplified API. |
| .claude/skills/review/SKILL.md | Updates internal review skill documentation (comparison-base aware). |
| .claude/skills/comparison-base/SKILL.md | Adds new internal skill for base resolution logic. |
| .claude/skills/api-changes/SKILL.md | Updates internal skill docs to use resolved comparison base. |
| .changeset/preserve-bubblebench-container-compatibility.md | Adds standalone changeset describing the BubbleBench contract adjustment. |
Files not reviewed (2)
- pnpm-lock.yaml: Generated file
- website/pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/dds/claims/README.md:29
- This README snippet refers to
claims.get(key), butkeyis not defined in the snippet. Using the literal key from thetrySetClaimcall makes the example self-contained.
if (confirmation.status === "Accepted") {
// This client successfully claimed the key.
} else if (confirmation.status === "AlreadyClaimed") {
// Lost the race; use claims.get(key) to read the winning value.
}
packages/dds/claims/src/claimsFactory.ts:90
- This usage snippet refers to
claims.get(key), butkeyis not defined in the snippet. Consider using the literal key used in the call above so the sample is self-contained.
* } else if (confirmation.status === "AlreadyClaimed") {
* // Another client claimed it first; use claims.get(key) to read the winning value.
* }
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (apiType !== undefined) { | ||
| const documentId = `${documentIdPrefix}${apiType.toLocaleLowerCase()}`; | ||
| const match = candidates.find((document) => document.id === documentId); |
| * const result = claims.trySetClaim("singleton-component", componentHandle); | ||
| * if (result.status === "AlreadyClaimed") { | ||
| * // Another client already claimed it; use result.currentValue. | ||
| * // Another client already claimed it; use claims.get(key) to read the winning value. | ||
| * } else if (result.status === "Pending") { |
|
|
||
| if (result.status === "AlreadyClaimed") { | ||
| // Another client already claimed it; use result.currentValue. | ||
| // Another client already claimed it; use claims.get(key) to read the winning value. |
| BubbleBench remains compatible with Fluid and native array containers | ||
|
|
||
| BubbleBench now uses a minimal shared collection contract so its implementations can provide either Tree arrays or native arrays without requiring the full built-in array interface. |
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
Description
BubbleBench needs to support both Fluid Tree arrays and native arrays. This PR introduces a minimal shared collection contract so the benchmark remains compatible with both container implementations without requiring the full built-in array interface.
Because the affected BubbleBench packages are private, the standalone changeset records the fix without adding package changelog entries, following the repository changeset guidance.
Validation performed:
git diff --check upstream/main...HEADbuild:compilefor all five BubbleBench packages:common,baseline,ot,experimental-tree, andshared-treeot,experimental-tree, andshared-tree(all passed)Reviewer Guidance
The review process is outlined on this wiki page.
The diff is limited to the shared BubbleBench collection type, its three Tree/OT adapters, and a standalone no-package changeset.