feat: replace meteor mongo driver with custom implementation - #1786
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces Meteor Mongo wrappers with native driver and change-stream layers. It migrates publication caches and server writes to replace-style persistence. It also adds local dev Mongo startup, a separate integration test project, and parity coverage for query, write, and observe behavior. ChangesCore Mongo primitives and collection wrappers
Publications, APIs, and persistence
Dev tooling, integration tests, and workspace alignment
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
09626e5 to
6a6fb5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
packages/corelib/src/__tests__/mongo.spec.ts (1)
410-512: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd projection edge cases for
_id-only exclusion and booleanfalse.The new projection tests do not cover
{ _id: 0 }by itself or{ field: false }, which are the edge cases inmongoProjectDocument/flattenProjectionmost likely to regress.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/__tests__/mongo.spec.ts` around lines 410 - 512, The mongoProjectDocument/flattenProjection tests are missing key projection edge cases for single-field exclusion and boolean syntax. Add coverage in mongo.spec for `{ _id: 0 }` by itself and for `{ field: false }` so both exclusion-only projection and boolean-false exclusion are verified alongside the existing include/exclude cases.meteor/__mocks__/mongo.ts (1)
34-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast when the backing mock collection is missing.
AsyncOnlyReadOnlyMongoCollectiondoes not exposemockCollection, so this can returnundefinedwhile typed asInMemoryMongoCollection<T>. Add an invariant error for accidental use with a native/readonly wrapper.Proposed guard
export function getInnerMockCollection<T extends { _id: ProtectedString<any> }>( collection: AsyncOnlyReadOnlyMongoCollection<T> ): InMemoryMongoCollection<T> { - return (collection as any).mockCollection + const mockCollection = (collection as any).mockCollection + if (!mockCollection) { + throw new Error(`Collection "${collection.name}" is not backed by an in-memory mock collection`) + } + return mockCollection }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/__mocks__/mongo.ts` around lines 34 - 39, The getInnerMockCollection helper currently assumes every AsyncOnlyReadOnlyMongoCollection has a mockCollection backing, which can silently return undefined despite the InMemoryMongoCollection<T> return type. Add a fail-fast invariant in getInnerMockCollection that checks the underlying mockCollection exists before returning it, and throw a clear error when the function is used with a native/readonly wrapper instead of a mock collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meteor/__mocks__/integration-global-setup.js`:
- Line 16: The MongoMemoryReplSet setup in integration-global-setup.js is still
using the default mongod binary version instead of the pinned release used
elsewhere. Update the MongoMemoryReplSet.create call to pass the same
binary.version value currently enforced by scripts/run.mjs (7.0.16) so the
integration replset matches the dev/conformance MongoDB version. Use the
existing replSet setup code path as the place to apply the version pin.
In `@meteor/server/__tests__/integration/changeStreamEngine.integration.test.ts`:
- Around line 89-127: The test only covers fan-out within a single
ObserveMultiplexer instance and does not verify cross-observer deduplication.
Update the integration test to drive two separate observe registrations through
the production dedup path so the shared-stream contract is exercised end-to-end.
Use the existing CollectionChangeFeed, ObserveMultiplexer, and
addObserveChangesSubscriber setup as a guide, but create independent observers
that go through the real dedup layer rather than sharing one multiplexer
instance.
In `@meteor/server/__tests__/integration/conformance/_harness.ts`:
- Around line 27-30: The shared harness’s throw cases are too loose because
Expectation.reason is never checked and inMemoryThrows does not confirm the real
Mongo path still succeeds. Update the expectation handling around Expectation,
especially the inMemoryThrows and bothThrow branches in the harness helpers, so
the thrown error must match the supplied reason and the non-failing backend is
explicitly verified to stay healthy; then adjust all affected call sites to
supply and assert the specific reason consistently.
In
`@meteor/server/__tests__/integration/conformance/observeParity.integration.test.ts`:
- Around line 52-80: The mock observer in runMock is not guaranteed to be
stopped if an insertAsync, updateAsync, replaceAsync, removeAsync, or
waitForCounts call throws, which can leak callbacks into later tests. Wrap the
post-observeChanges body in a try/finally and always call handle.stop() in the
finally block, mirroring the cleanup pattern used by runReal and the
observeChanges handle setup in runMock. Also apply the same cleanup to the
related observe/observer block in the other affected test section so both mock
observer lifecycles are always terminated.
In `@meteor/server/collections/changeStream/__tests__/observeMultiplexer.test.ts`:
- Around line 62-68: The test cleanup only aborts the shared observers
controller, but `joinChangesFeed` also registers separate controllers that can
leak into the global multiplexer registry if a test exits early. Update the
`observeMultiplexer.test.ts` cleanup around `beforeEach`/`afterEach` and any
test-specific teardown sites to also track and abort those registry subscribers
created by `joinChangesFeed`, so every controller is disposed even when
assertions fail. Use the existing `observers` setup and the `joinChangesFeed`
helper as the main points to locate and clean up all subscriptions.
In `@meteor/server/collections/changeStream/changeStreamCursor.ts`:
- Around line 19-20: The cursor counting path is not honoring the applySkipLimit
flag, so countAsync(false) still uses the limited count. Update the count
contract in changeStreamCursor and thread the flag through countAsync so it
forwards the config to the underlying count call, then adjust
WrappedAsyncMongoCollection.findWithCursor to omit skip/limit when the flag is
false. Use the existing countAsync, count, and findWithCursor symbols to make
the change consistent across the cursor and collection wrapper.
In `@meteor/server/collections/changeStream/observeMultiplexer.ts`:
- Around line 115-124: The current `#enqueue` method in ObserveMultiplexer
swallows all task errors, which lets an initial `#sync` failure be treated like a
successful setup and allows addSubscriber to resolve a live observer against an
empty or stale snapshot. Update the startup path so snapshot/setup failures are
surfaced back to the observe caller instead of being logged and ignored, while
preserving the log-and-continue behavior only for ongoing live change-event
processing; use the `#enqueue`, `#sync`, and addSubscriber flow to distinguish
initial replay/snapshot work from later event tasks.
- Around line 309-319: If the AbortSignal is already aborted, return immediately
before calling getOrCreateMultiplexer in observeChangesViaChangeStream so no
multiplexer/collection feed is allocated for a request that will never
subscribe. Add the same early-abort guard to the other observe helper mentioned
in the comment, using the existing signal parameter and the relevant
addSubscriber/addObserveChangesSubscriber flow, so aborted calls never create a
zero-subscriber multiplexer.
In `@meteor/server/collections/implementations/asyncCollection.ts`:
- Around line 365-377: `bulkWriteAsync` should end its tracing span and
normalize driver failures the same way as the other native-driver methods in
`AsyncCollection`. Wrap the `_rawCollection.bulkWrite()` call and the
write-error check in a `try/finally` so `span.end()` always runs, and route any
rejection through `wrapMongoError` before rethrowing. Use the `bulkWriteAsync`
method and the `span` handling in `AsyncCollection` as the places to update.
In `@meteor/server/collections/implementations/mock.ts`:
- Around line 99-101: The mock cursor’s countAsync implementation in
MinimalMongoCursor currently ignores the optional false argument and always
counts the captured cursor state, so it does not match countAsync(false)
behavior. Update the countAsync method on the mock cursor to accept and honor
the boolean parameter, and when false is passed make it count without applying
skip/limit while preserving the existing behavior for the default case; use
MinimalMongoCursor and its countAsync/cursor logic to locate the change.
- Around line 71-130: The mock collection wrapper is bypassing Meteor-style
field normalization, so `findFetchAsync`, `findOneAsync`, `findWithCursor`,
`observe`, and `observeChanges` can behave differently from production. Update
the mock in `MockCollection` to normalize any passed `fields` into `projection`
before calling `#core.findFetch`, `#core.findOne`, `#core.find`, and the
cursor/observe helpers, matching the behavior already implemented in the real
wrapper.
In `@meteor/server/collections/mongoConnection.ts`:
- Around line 37-39: Do not let isInMockMode() switch to mock mode solely
because process.env.MONGO_URL is missing; make the decision depend only on
explicit test/mock signals like Meteor.isTest or JEST_WORKER_ID, and ensure the
startup path in mongoConnection.ts still performs the real connection and
fail-fast behavior when not in an explicit mock/test environment. Update the
logic around isInMockMode() and the startup connection block so production-like
runs cannot silently fall back to in-memory collections.
In `@meteor/server/publications/lib/lib.ts`:
- Around line 59-71: Register the stop handler in driveSubscriptionFromCursor
before awaiting cursor.observeChangesAsync(), so unsubscribes during startup can
still tear down the observer. Update the startup flow around
observeChangesAsync, SubscriptionContext.onStop, and the ChangeStreamCursor API
so an abort/stop signal or handle can be passed into the async initialization
and cancelled if sub stops before the promise resolves.
- Around line 76-89: The callback contract in the public publication helper is
too broad: the runtime path in the publish wrapper only accepts a
ChangeStreamCursor, but the signature still allows any MinimalMongoCursor.
Update the callback return type in the publication API around the publish helper
to require ChangeStreamCursor<PublishDocType<K>> | null (or an equivalent
publishable-cursor type), and keep the runtime instanceof ChangeStreamCursor
guard in sync so unsupported cursors fail at compile time instead of only in the
Meteor publish wrapper.
In `@packages/corelib/src/collectionChangeFeed.ts`:
- Around line 112-118: Late subscribers are added in subscribe() but only
receive the initial snapshot from `#attachStream`(), so subscribers that join
while `#stream` is already open miss the current collection state. Update
CollectionChangeFeed.subscribe() (and any related attach/reconnect path around
`#attachStream`()) to trigger onResync for newly added subscribers when an active
stream already exists, while keeping existing behavior for the first subscriber
and reconnects.
In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts`:
- Around line 256-286: The observe/live-query path is ignoring cursor shaping
options, so `observe()` and `observeChanges()` currently publish an unbounded
match set instead of the cursor’s `{ sort, skip, limit }` window. Update the
`InMemoryMongoCollection` observe flow—especially `observe()`,
`observeChanges()`, and `_startObserve()`—to carry the full cursor shape into
`ObserveView`/snapshot seeding rather than only `projectionOf(this.#options)` or
`#findMatching(selector)`, and make sure the active top-N set is re-evaluated on
writes using the same ordering and bounds as the cursor.
- Around line 222-231: The `replaceOne` branch in `InMemoryMongoCollection` only
keys off `_id`, so it ignores any अतिरिक्त filter predicates and can replace
documents that no longer match the full query. Update the `replaceOne` handling
to resolve the target via the actual filter match (not just `filter._id`), and
only call `this.replace(...)` when the stored document satisfies the complete
`op.replaceOne.filter` criteria; keep the `upsert` path separate for
insert-on-miss behavior. Use the existing `replaceOne`, `replace`, and
`#documents` logic to locate the change.
In `@packages/corelib/src/mongo.ts`:
- Around line 224-229: The dotted-path handling in mongoWhere only recurses into
plain objects, so selectors like items.name fail when the path passes through an
array. Update the logic in mongoWhere to detect array values at each segment and
evaluate the remaining selector against each element, succeeding if any element
matches. Keep the existing object recursion path for non-array nested documents
and preserve the current handling for leaf selectors.
- Around line 360-386: Update mongoProjectDocument() so an explicit {_id: 0} is
treated as an exclusion projection and the returned document has _id removed
even when no other keys are present. Also fix flattenProjection() to preserve
boolean projection semantics by mapping true/1 to include and false/0 to
exclude, so fields specified with false are handled as exclusions rather than
includes.
In `@packages/webui/src/client/collections/types.ts`:
- Around line 48-58: The `update()` overloads in `types.ts` are too strict for
the runtime behavior in `WrappedMongoCollection.update`, because they require
`options.multi` for any `MongoQuery` selector even though the wrapper still
forwards single-document updates without it. Relax the public signature so
non-`_id` selectors remain valid without forcing `multi`, and keep the overloads
aligned with the implementation in `WrappedMongoCollection.update` and the
`UpdateOptions` contract.
---
Nitpick comments:
In `@meteor/__mocks__/mongo.ts`:
- Around line 34-39: The getInnerMockCollection helper currently assumes every
AsyncOnlyReadOnlyMongoCollection has a mockCollection backing, which can
silently return undefined despite the InMemoryMongoCollection<T> return type.
Add a fail-fast invariant in getInnerMockCollection that checks the underlying
mockCollection exists before returning it, and throw a clear error when the
function is used with a native/readonly wrapper instead of a mock collection.
In `@packages/corelib/src/__tests__/mongo.spec.ts`:
- Around line 410-512: The mongoProjectDocument/flattenProjection tests are
missing key projection edge cases for single-field exclusion and boolean syntax.
Add coverage in mongo.spec for `{ _id: 0 }` by itself and for `{ field: false }`
so both exclusion-only projection and boolean-false exclusion are verified
alongside the existing include/exclude cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd60eba2-5f0f-4dfb-aff6-4b31094cd5fe
⛔ Files ignored due to path filters (2)
meteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpackages/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (127)
.env.example.gitignoremeteor/.meteor/packagesmeteor/.meteor/versionsmeteor/__mocks__/_setupMocks.tsmeteor/__mocks__/helpers/lib.tsmeteor/__mocks__/integration-global-setup.jsmeteor/__mocks__/integration-global-teardown.jsmeteor/__mocks__/meteor-ejson.tsmeteor/__mocks__/mongo.tsmeteor/__mocks__/tracker.tsmeteor/jest.config.jsmeteor/package.jsonmeteor/scripts/dev-mongo.mjsmeteor/server/__tests__/_testEnvironment.test.tsmeteor/server/__tests__/integration/_integrationDb.tsmeteor/server/__tests__/integration/_observeHelpers.tsmeteor/server/__tests__/integration/changeStreamEngine.integration.test.tsmeteor/server/__tests__/integration/conformance/_harness.tsmeteor/server/__tests__/integration/conformance/bulkWrite.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoFindOptions.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoModify.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoWhere.integration.test.tsmeteor/server/__tests__/integration/conformance/observeParity.integration.test.tsmeteor/server/__tests__/integration/conformance/wrapperParity.integration.test.tsmeteor/server/api/blueprints/api.tsmeteor/server/api/deviceTriggers/StudioDeviceTriggerManager.tsmeteor/server/api/deviceTriggers/StudioObserver.tsmeteor/server/api/deviceTriggers/__tests__/TagsService.test.tsmeteor/server/api/deviceTriggers/observer.tsmeteor/server/api/deviceTriggers/reactiveContentCache.tsmeteor/server/api/deviceTriggers/reactiveContentCacheForPieceInstances.tsmeteor/server/api/integration/expectedPackages.tsmeteor/server/api/integration/media-scanner.tsmeteor/server/api/mongo.tsmeteor/server/api/rest/v1/showstyles.tsmeteor/server/api/rest/v1/studios.tsmeteor/server/api/rundownLayouts.tsmeteor/server/api/system.tsmeteor/server/api/translationsBundles.tsmeteor/server/api/triggeredActions.tsmeteor/server/collections/changeStream/__tests__/observeMultiplexer.test.tsmeteor/server/collections/changeStream/changeStreamCursor.tsmeteor/server/collections/changeStream/collectionChangeFeed.tsmeteor/server/collections/changeStream/observeMultiplexer.tsmeteor/server/collections/collection.tsmeteor/server/collections/implementations/asyncCollection.tsmeteor/server/collections/implementations/mock.tsmeteor/server/collections/implementations/readonlyWrapper.tsmeteor/server/collections/indices.tsmeteor/server/collections/mongoConnection.tsmeteor/server/lib/customPublication/customPublishCollection.tsmeteor/server/lib/customPublication/optimizedObserverArray.tsmeteor/server/lib/customPublication/optimizedObserverBase.tsmeteor/server/lib/database.tsmeteor/server/migration/26_03.tsmeteor/server/migration/lib.tsmeteor/server/migration/upgrades/lib.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/blueprintUpgradeStatus/reactiveContentCache.tsmeteor/server/publications/ingestStatus/createIngestRundownStatus.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/ingestStatus/reactiveContentCache.tsmeteor/server/publications/ingestStatus/rundownContentObserver.tsmeteor/server/publications/lib/ReactiveCacheCollection.tsmeteor/server/publications/lib/__tests__/rundownsObserver.test.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/lib/observerChain.tsmeteor/server/publications/lib/quickLoop.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/packageManager/expectedPackages/contentCache.tsmeteor/server/publications/packageManager/expectedPackages/contentObserver.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partInstancesUI/reactiveContentCache.tsmeteor/server/publications/partInstancesUI/rundownContentObserver.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/partsUI/reactiveContentCache.tsmeteor/server/publications/partsUI/rundownContentObserver.tsmeteor/server/publications/pieceContentStatusUI/__tests__/checkPieceContentStatus.test.tsmeteor/server/publications/pieceContentStatusUI/bucket/bucketContentCache.tsmeteor/server/publications/pieceContentStatusUI/bucket/bucketContentObserver.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/reactiveContentCache.tsmeteor/server/publications/pieceContentStatusUI/rundown/rundownContentObserver.tsmeteor/server/publications/segmentPartNotesUI/__tests__/publication.test.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/segmentPartNotesUI/reactiveContentCache.tsmeteor/server/worker/worker.tspackage.jsonpackages/corelib/package.jsonpackages/corelib/src/__tests__/collectionChangeFeed.spec.tspackages/corelib/src/__tests__/diffObject.spec.tspackages/corelib/src/__tests__/mongo.spec.tspackages/corelib/src/collectionChangeFeed.tspackages/corelib/src/diffObject.tspackages/corelib/src/memoryCollection/InMemoryMongoCollection.tspackages/corelib/src/memoryCollection/__tests__/InMemoryMongoCollection.spec.tspackages/corelib/src/memoryCollection/__tests__/observeView.spec.tspackages/corelib/src/memoryCollection/index.tspackages/corelib/src/memoryCollection/observeView.tspackages/corelib/src/mongo.tspackages/documentation/docs/user-guide/installation/quick-install.mdpackages/job-worker/src/__mocks__/collection.tspackages/job-worker/src/__tests__/rundownPlaylist.test.tspackages/job-worker/src/db/changes.tspackages/job-worker/src/db/collection.tspackages/job-worker/src/db/collections.tspackages/job-worker/src/ingest/commit.tspackages/job-worker/src/ingest/expectedPackages.tspackages/job-worker/src/ingest/model/implementation/DocumentChangeTracker.tspackages/job-worker/src/ingest/model/implementation/SaveIngestModel.tspackages/job-worker/src/ingest/nrcsIngestCache.tspackages/job-worker/src/ingest/sofieIngestCache.tspackages/job-worker/src/notifications/NotificationsModelHelper.tspackages/job-worker/src/playout/expectedPackages.tspackages/job-worker/src/playout/model/implementation/SavePlayoutModel.tspackages/job-worker/src/playout/model/implementation/__tests__/LoadPlayoutModel.spec.tspackages/job-worker/src/playout/timings/timelineTriggerTime.tspackages/job-worker/src/updatePartInstanceRanksAndOrphanedState.tspackages/meteor-lib/src/collections/lib.tspackages/package.jsonpackages/webui/src/__mocks__/mongo.tspackages/webui/src/client/collections/lib.tspackages/webui/src/client/collections/types.tsscripts/run.mjs
💤 Files with no reviewable changes (5)
- meteor/.meteor/packages
- meteor/server/publications/lib/ReactiveCacheCollection.ts
- meteor/mocks/tracker.ts
- meteor/mocks/helpers/lib.ts
- meteor/.meteor/versions
6a6fb5c to
0e4b681
Compare
0e4b681 to
b564842
Compare
expand it to support nested objects as the types claim
this covers crud and observing changes
with support for observing changes similar to proper collections
b564842 to
f31a1de
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meteor/server/collections/implementations/asyncCollection.ts (1)
180-205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
nonMutatingCallbacksfrom the public observe options.The collection interface now passes a single
FindObserveChangesOptionsobject, but this implementation only checkscallbackOptions?.nonMutatingCallbacks. Calls through the interface with{ nonMutatingCallbacks: true }will always use mutating callback cloning.Suggested fix
async observeChanges( selector: MongoQuery<DBInterface> | DBInterface['_id'], callbacks: PromisifyCallbacks<ObserveChangesCallbacks<DBInterface>>, - findOptions?: FindOptions<DBInterface>, - callbackOptions?: ObserveChangesOptions + options?: FindObserveChangesOptions<DBInterface> ): Promise<Meteor.LiveQueryHandle> { @@ await observeChangesViaChangeStream( this.name, sel, - this.projectionOf(findOptions), - this.shapeOf(findOptions), + this.projectionOf(options), + this.shapeOf(options), callbacks, abort.signal, - !!callbackOptions?.nonMutatingCallbacks, + !!options?.nonMutatingCallbacks, () => this.observeDeps(sel) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/collections/implementations/asyncCollection.ts` around lines 180 - 205, The observeChanges implementation is only reading nonMutatingCallbacks from the internal callbackOptions argument, so public callers passing FindObserveChangesOptions do not get honored. Update asyncCollection.observeChanges to read nonMutatingCallbacks from the options object used by the collection interface (while keeping backward compatibility if needed), and pass that value into observeChangesViaChangeStream instead of relying solely on callbackOptions?.nonMutatingCallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts`:
- Around line 344-350: The changed handler in InMemoryMongoCollection is still
sending undefined-valued keys through `$set` because it uses `omit(fields,
'_id')` directly, so fields meant to be cleared can remain present. Update the
`changed` callback to build the `$set` payload from only defined entries in
`fields` while keeping the existing `$unset` construction for keys whose value
is `undefined`. Keep the fix localized to the `changed` method in
`InMemoryMongoCollection` and ensure `_id` is still excluded from `$set`.
- Around line 167-174: The update path in InMemoryMongoCollection.update()
currently allows mongoModify() to change _id and then re-keys `#documents`, which
can overwrite another entry. Update the logic so _id is treated as immutable:
after calling mongoModify(sel, clone(doc), modifier), preserve the original
doc._id as the storage key and ignore or reject any changed _id in the modified
document. Keep the existing update flow and `#emit` behavior intact, but ensure
the document is always stored back under the original key.
In `@packages/corelib/src/memoryCollection/observeView.ts`:
- Around line 101-109: `applySnapshot` and the windowed path in `observeView.ts`
are projecting documents too early, so `#matching` loses sort fields before
`#reconcileWindow()` runs. Keep full documents in the windowed `#matching` map
until after ordering/top-N selection is computed, then apply
`mongoProjectDocument` only when publishing results; update the related window
reconciliation flow (including the other `#matching` usages around the noted
methods) so sorting always uses unprojected docs.
---
Outside diff comments:
In `@meteor/server/collections/implementations/asyncCollection.ts`:
- Around line 180-205: The observeChanges implementation is only reading
nonMutatingCallbacks from the internal callbackOptions argument, so public
callers passing FindObserveChangesOptions do not get honored. Update
asyncCollection.observeChanges to read nonMutatingCallbacks from the options
object used by the collection interface (while keeping backward compatibility if
needed), and pass that value into observeChangesViaChangeStream instead of
relying solely on callbackOptions?.nonMutatingCallbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1ab51ce-034a-4f93-978b-68f38fec7497
⛔ Files ignored due to path filters (2)
meteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpackages/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (99)
.env.example.gitignoremeteor/.meteor/packagesmeteor/.meteor/versionsmeteor/__mocks__/_setupMocks.tsmeteor/__mocks__/integration-global-setup.jsmeteor/__mocks__/integration-global-teardown.jsmeteor/__mocks__/meteor-ejson.tsmeteor/__mocks__/mongo.tsmeteor/__mocks__/tracker.tsmeteor/jest.config.jsmeteor/package.jsonmeteor/scripts/dev-mongo.mjsmeteor/server/__tests__/_testEnvironment.test.tsmeteor/server/__tests__/integration/_integrationDb.tsmeteor/server/__tests__/integration/_observeHelpers.tsmeteor/server/__tests__/integration/changeStreamEngine.integration.test.tsmeteor/server/__tests__/integration/conformance/_harness.tsmeteor/server/__tests__/integration/conformance/bulkWrite.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoFindOptions.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoModify.integration.test.tsmeteor/server/__tests__/integration/conformance/mongoWhere.integration.test.tsmeteor/server/__tests__/integration/conformance/observeParity.integration.test.tsmeteor/server/__tests__/integration/conformance/wrapperParity.integration.test.tsmeteor/server/api/deviceTriggers/StudioDeviceTriggerManager.tsmeteor/server/api/deviceTriggers/StudioObserver.tsmeteor/server/api/deviceTriggers/TagsService.tsmeteor/server/api/deviceTriggers/__tests__/TagsService.test.tsmeteor/server/api/deviceTriggers/observer.tsmeteor/server/api/deviceTriggers/reactiveContentCache.tsmeteor/server/api/deviceTriggers/reactiveContentCacheForPieceInstances.tsmeteor/server/api/mongo.tsmeteor/server/api/system.tsmeteor/server/collections/changeStream/__tests__/observeMultiplexer.test.tsmeteor/server/collections/changeStream/changeStreamCursor.tsmeteor/server/collections/changeStream/collectionChangeFeed.tsmeteor/server/collections/changeStream/observeMultiplexer.tsmeteor/server/collections/collection.tsmeteor/server/collections/implementations/asyncCollection.tsmeteor/server/collections/implementations/mock.tsmeteor/server/collections/implementations/readonlyWrapper.tsmeteor/server/collections/indices.tsmeteor/server/collections/mongoConnection.tsmeteor/server/lib/customPublication/customPublishCollection.tsmeteor/server/lib/customPublication/optimizedObserverArray.tsmeteor/server/lib/customPublication/optimizedObserverBase.tsmeteor/server/migration/26_03.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/blueprintUpgradeStatus/reactiveContentCache.tsmeteor/server/publications/ingestStatus/createIngestRundownStatus.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/ingestStatus/reactiveContentCache.tsmeteor/server/publications/ingestStatus/rundownContentObserver.tsmeteor/server/publications/lib/ReactiveCacheCollection.tsmeteor/server/publications/lib/__tests__/rundownsObserver.test.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/lib/observerChain.tsmeteor/server/publications/lib/quickLoop.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/organization.tsmeteor/server/publications/packageManager/expectedPackages/contentCache.tsmeteor/server/publications/packageManager/expectedPackages/contentObserver.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partInstancesUI/reactiveContentCache.tsmeteor/server/publications/partInstancesUI/rundownContentObserver.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/partsUI/reactiveContentCache.tsmeteor/server/publications/partsUI/rundownContentObserver.tsmeteor/server/publications/pieceContentStatusUI/__tests__/checkPieceContentStatus.test.tsmeteor/server/publications/pieceContentStatusUI/bucket/bucketContentCache.tsmeteor/server/publications/pieceContentStatusUI/bucket/bucketContentObserver.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/reactiveContentCache.tsmeteor/server/publications/pieceContentStatusUI/rundown/rundownContentObserver.tsmeteor/server/publications/segmentPartNotesUI/__tests__/publication.test.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/segmentPartNotesUI/reactiveContentCache.tsmeteor/server/worker/worker.tspackage.jsonpackages/corelib/package.jsonpackages/corelib/src/__tests__/collectionChangeFeed.spec.tspackages/corelib/src/__tests__/diffObject.spec.tspackages/corelib/src/__tests__/mongo.spec.tspackages/corelib/src/collectionChangeFeed.tspackages/corelib/src/diffObject.tspackages/corelib/src/memoryCollection/InMemoryMongoCollection.tspackages/corelib/src/memoryCollection/__tests__/InMemoryMongoCollection.spec.tspackages/corelib/src/memoryCollection/__tests__/observeView.spec.tspackages/corelib/src/memoryCollection/index.tspackages/corelib/src/memoryCollection/observeView.tspackages/corelib/src/mongo.tspackages/documentation/docs/user-guide/installation/quick-install.mdpackages/meteor-lib/src/collections/lib.tspackages/webui/src/__mocks__/mongo.tspackages/webui/src/client/collections/lib.tspackages/webui/src/client/collections/types.tsscripts/run.mjs
💤 Files with no reviewable changes (4)
- meteor/mocks/tracker.ts
- meteor/server/publications/lib/ReactiveCacheCollection.ts
- meteor/.meteor/versions
- meteor/.meteor/packages
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- meteor/server/publications/lib/observerChain.ts
- meteor/server/tests/integration/conformance/mongoFindOptions.integration.test.ts
- meteor/server/api/deviceTriggers/StudioObserver.ts
- .env.example
- meteor/server/publications/lib/tests/rundownsObserver.test.ts
- meteor/server/lib/customPublication/optimizedObserverArray.ts
🚧 Files skipped from review as they are similar to previous changes (61)
- packages/corelib/package.json
- package.json
- meteor/mocks/integration-global-teardown.js
- meteor/server/api/system.ts
- meteor/server/collections/indices.ts
- meteor/server/api/deviceTriggers/observer.ts
- meteor/server/api/deviceTriggers/tests/TagsService.test.ts
- meteor/mocks/integration-global-setup.js
- meteor/server/worker/worker.ts
- meteor/package.json
- meteor/server/tests/integration/_integrationDb.ts
- meteor/server/lib/customPublication/customPublishCollection.ts
- meteor/server/tests/integration/conformance/bulkWrite.integration.test.ts
- meteor/server/lib/customPublication/optimizedObserverBase.ts
- packages/corelib/src/tests/diffObject.spec.ts
- packages/corelib/src/memoryCollection/index.ts
- meteor/server/collections/changeStream/collectionChangeFeed.ts
- packages/documentation/docs/user-guide/installation/quick-install.md
- meteor/server/publications/packageManager/expectedPackages/contentCache.ts
- meteor/server/publications/partInstancesUI/reactiveContentCache.ts
- meteor/scripts/dev-mongo.mjs
- meteor/server/tests/integration/changeStreamEngine.integration.test.ts
- packages/corelib/src/tests/collectionChangeFeed.spec.ts
- meteor/server/publications/partsUI/reactiveContentCache.ts
- meteor/server/tests/integration/conformance/mongoModify.integration.test.ts
- meteor/server/publications/pieceContentStatusUI/rundown/reactiveContentCache.ts
- meteor/server/api/deviceTriggers/reactiveContentCache.ts
- packages/corelib/src/diffObject.ts
- meteor/server/publications/segmentPartNotesUI/tests/publication.test.ts
- meteor/server/publications/lib/quickLoop.ts
- meteor/server/publications/ingestStatus/publication.ts
- meteor/server/tests/integration/conformance/observeParity.integration.test.ts
- meteor/server/tests/integration/conformance/mongoWhere.integration.test.ts
- meteor/server/collections/mongoConnection.ts
- meteor/server/publications/pieceContentStatusUI/bucket/bucketContentCache.ts
- meteor/server/tests/integration/conformance/_harness.ts
- meteor/mocks/_setupMocks.ts
- meteor/server/publications/blueprintUpgradeStatus/reactiveContentCache.ts
- meteor/server/api/mongo.ts
- meteor/server/publications/pieceContentStatusUI/tests/checkPieceContentStatus.test.ts
- meteor/server/collections/implementations/readonlyWrapper.ts
- meteor/server/tests/integration/conformance/wrapperParity.integration.test.ts
- meteor/server/tests/_testEnvironment.test.ts
- packages/meteor-lib/src/collections/lib.ts
- meteor/mocks/mongo.ts
- packages/corelib/src/collectionChangeFeed.ts
- meteor/server/migration/26_03.ts
- meteor/server/publications/partInstancesUI/rundownContentObserver.ts
- packages/webui/src/client/collections/types.ts
- meteor/server/tests/integration/_observeHelpers.ts
- meteor/server/api/deviceTriggers/reactiveContentCacheForPieceInstances.ts
- packages/webui/src/mocks/mongo.ts
- meteor/server/publications/segmentPartNotesUI/reactiveContentCache.ts
- meteor/server/publications/partsUI/rundownContentObserver.ts
- scripts/run.mjs
- meteor/server/collections/changeStream/tests/observeMultiplexer.test.ts
- meteor/jest.config.js
- meteor/server/publications/ingestStatus/reactiveContentCache.ts
- packages/webui/src/client/collections/lib.ts
- meteor/server/publications/lib/lib.ts
- packages/corelib/src/mongo.ts
f31a1de to
25d387c
Compare
25d387c to
f00bb83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts (3)
305-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegister the change listener before seeding the initial snapshot.
Line 315 sends the initial
addedcallbacks. Without a scheduler, delivery can be synchronous. If anaddedcallback writes to this collection,#emitruns before Line 320 registers the listener, so the observer misses that write and becomes stale.Register the listener before
applySnapshot, or buffer and replay events during setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts` around lines 305 - 320, The observer listener is registered after the initial snapshot, allowing synchronous writes from `view.applySnapshot` callbacks to be missed. In the observer setup flow, create and add the `listener` to `this.#listeners` before calling `view.applySnapshot(this.#findMatching(selector))`, while preserving the existing event forwarding behavior.
225-230: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInsert on a replace-upsert miss.
When
matchingis empty,this.replace(...)still looks upnewIdby_id. If that ID belongs to a document that failed an extra filter predicate, this branch overwrites that document.Use
this.insert(...)so an existing ID produces a duplicate-key error instead of replacing unrelated data.Proposed fix
- this.replace({ ...replacement, _id: newId } as TDoc) + this.insert({ ...replacement, _id: newId } as TDoc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts` around lines 225 - 230, In the replace-upsert miss branch of replaceOne, change the write operation from this.replace(...) to this.insert(...), preserving the existing replacement and _id derivation so duplicate IDs raise an error instead of overwriting a document excluded by the filter.
322-327: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSuppress scheduled observer callbacks on
stop().
makeObserveSinkandmakeObserveChangesSinkschedule callbacks throughObserverDeliveryScheduler, butstop()only removes#listenersand theobserversentry. A scheduled callback can still run after the observer is stopped. Track stopped callbacks in the sinks/handles and ignore delivery afterstop()is called.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts` around lines 322 - 327, Update makeObserveSink and makeObserveChangesSink so each returned handle tracks whether it has been stopped, and ensure scheduled ObserverDeliveryScheduler callbacks check that state before delivering. Set the stopped state before removing the listener and observer entry in stop(), preventing any already-scheduled callback from running afterward.
♻️ Duplicate comments (1)
packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts (1)
342-348: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep undefined fields out of
$set.The loop adds undefined values to
$unset, but Line 348 also passes those values throughomit(fields, '_id')in$set. The final state can then depend on modifier order and leave cleared fields present.Build
$setonly from defined entries.Proposed fix
changed: (id, fields) => { const unset: Partial<Record<keyof TDoc, 1>> = {} + const set: Partial<TDoc> = {} for (const [key, value] of Object.entries<unknown>(fields as Record<string, unknown>)) { - if (value !== undefined) continue - unset[key as keyof TDoc] = 1 + if (key === '_id') continue + if (value === undefined) { + unset[key as keyof TDoc] = 1 + } else { + ;(set as Record<string, unknown>)[key] = value + } } - this.update(id, { $set: omit(fields, '_id') as any, $unset: unset as any }) + this.update(id, { $set: set as any, $unset: unset as any })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts` around lines 342 - 348, Update the changed callback’s update construction to exclude undefined-valued entries from $set while retaining them in $unset. Build the $set payload from the filtered fields object rather than passing the original fields through omit, and preserve the existing _id exclusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts`:
- Around line 45-47: Update the JSDoc for the idGenerator option in
InMemoryMongoCollectionOptions to name getRandomId as the default, matching the
constructor implementation. Leave the option behavior unchanged.
---
Outside diff comments:
In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts`:
- Around line 305-320: The observer listener is registered after the initial
snapshot, allowing synchronous writes from `view.applySnapshot` callbacks to be
missed. In the observer setup flow, create and add the `listener` to
`this.#listeners` before calling
`view.applySnapshot(this.#findMatching(selector))`, while preserving the
existing event forwarding behavior.
- Around line 225-230: In the replace-upsert miss branch of replaceOne, change
the write operation from this.replace(...) to this.insert(...), preserving the
existing replacement and _id derivation so duplicate IDs raise an error instead
of overwriting a document excluded by the filter.
- Around line 322-327: Update makeObserveSink and makeObserveChangesSink so each
returned handle tracks whether it has been stopped, and ensure scheduled
ObserverDeliveryScheduler callbacks check that state before delivering. Set the
stopped state before removing the listener and observer entry in stop(),
preventing any already-scheduled callback from running afterward.
---
Duplicate comments:
In `@packages/corelib/src/memoryCollection/InMemoryMongoCollection.ts`:
- Around line 342-348: Update the changed callback’s update construction to
exclude undefined-valued entries from $set while retaining them in $unset. Build
the $set payload from the filtered fields object rather than passing the
original fields through omit, and preserve the existing _id exclusion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb1d3bef-c63b-492e-b433-d889ea0c6c45
📒 Files selected for processing (2)
packages/corelib/src/memoryCollection/InMemoryMongoCollection.tspackages/corelib/src/memoryCollection/__tests__/InMemoryMongoCollection.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/corelib/src/memoryCollection/tests/InMemoryMongoCollection.spec.ts
# Conflicts: # meteor/__mocks__/_setupMocks.ts # meteor/__mocks__/mongo.ts # meteor/package.json # meteor/server/collections/implementations/asyncCollection.ts # meteor/server/collections/implementations/mock.ts # meteor/server/publications/lib/lib.ts # meteor/server/publications/mountedTriggers.ts # meteor/server/publications/organization.ts # meteor/yarn.lock
jest only reads `testTimeout` from the global config; setting it on a project inside `projects: [...]` is silently ignored. Every integration test without an inline timeout was therefore running with jest's 5s default while the config claimed 20s - far too tight for real change-stream and replica-set I/O, which is the main cause of the intermittent failures. Raised via `jest.setTimeout` in a setup file on the integration project, which does apply, leaving the unit project fast. The observeParity `test.each` also passed an explicit 15000, exactly equal to `waitFor`'s default ceiling, so jest always won the race and reported a bare "Exceeded timeout" instead of the helper's message. Dropped, and `waitFor` now reports what it was still waiting for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The engine test's waits now say what they were still waiting for, which is what identified the residual intermittent failure as a stale-snapshot revert (published val goes 1 -> 2 -> 1 -> 2 for a single update) rather than slow delivery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This makes it match what the driver and our mock supports, and removes some meteor specifics
About the Contributor
This pull request is posted on behalf of Superfly
This is part of a series of PRs aiming to replace meteor.
This builds upon #1782
Type of Contribution
This is a: Feature / Code improvement
Current Behavior
We are relying on meteor for a mongodb driver, which uses oplog tailing. While this works fine for us, to be able to leave meteor we need a non-meteor implementation of this.
New Behavior
This makes a few key changes
Some chunks have been written in corelib as they are not 'meteor' specific, and could be useful in other places in the future.
Replace the meteor spawned mongodb with our own using
mongodb-memory-serverWhile not essential at this stage, it gives some slightly better DX by allowing us to choose the version used, choose the port easier and potentially the storage path.
For now it is defaulting to the same as meteor ran, including the same path. This makes it easy to switch in and out of this branch; as long as everything is stopped when switching, the db is kept compatible and in the same place.
As part of this, we have dropped the
meteor/mongopackage entirely, and it will no longer be installed.env file support
To make it easier for configuring the spawned mongodb, we now load a
.envfile of environment variables. This allows for defining clone/worktree specific settings. For now only things about mongo, but this can be expanded in the future.A new in-memory collection implementation
For a few publications we have been using a
meteor/mongominimongo collection. A new implementation has been rebuilt using the mongo query/mutation utils we have for our unit test mocks.The test mocks have also been rewritten to use this collection.
Changes stream watcher
A new changes stream watcher has been built to be the source of all collection watching changes. I believe that change streams do not report anything when a document falls out of its query, and there is an unknown question about performance if setting up dozens of watchers, so we are setting up a single watcher per collection (lazily).
Then inside the app we are figuring out which
observecalls a change should actually route to.While this feels like a lot of wasted work in many cases, I believe this is exactly what meteor was doing, so it should be no different in performance.
Some sprawling changes
As part of that in-memory collection and rebuilt observing, there have been some small types changes which have to be spread around various callsites in the publications logic. This makes for a sprawling change, but a lot of the files are just a few simple changes.
Testing
Affected areas
Time Frame
Other Information
Status