feat: refactor ddp publication registration into a PublicationRegistry#1785
feat: refactor ddp publication registration into a PublicationRegistry#1785Julusian wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (38)
WalkthroughReplaces import-time Meteor registration with explicit method and publication registries, exports server API classes with typed async signatures, moves publications behind registry entry points, and updates startup, REST routing, verification, and tests to use the new wiring. ChangesRegistry-based Meteor method and publication registration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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! |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
meteor/server/api/client.ts (1)
442-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this API method return type explicit.
This exported registry surface still suppresses explicit boundary typing while neighboring API methods declare their
Promise<...>return types.Proposed cleanup
- // eslint-disable-next-line `@typescript-eslint/explicit-module-boundary-types` async callPeripheralDeviceFunction( context: string, deviceId: PeripheralDeviceId, timeoutTime: number | undefined, functionName: string, ...args: any[] - ) { + ): Promise<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 `@meteor/server/api/client.ts` around lines 442 - 449, The exported API method callPeripheralDeviceFunction still relies on explicit-boundary suppression instead of declaring its return type like nearby registry methods. Remove the boundary-typing suppression and add an explicit Promise-based return type on callPeripheralDeviceFunction that matches its resolved value, keeping the registry surface consistently typed with the neighboring API methods.
🤖 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/server/__tests__/methodRegistry.test.ts`:
- Around line 26-28: The drift guard in the MethodRegistry test is relying on a
hard-coded name prefix to skip debug methods. Update the loop in
methodRegistry.test.ts to use MethodRegistry.isDebugMethod(name) instead of
startsWith('debug'), so the test follows the registry’s source-of-truth debug
detection and stays valid if debug method names change.
In `@meteor/server/api/rest/v0/index.ts`:
- Around line 78-79: The REST route builder is using raw publication signatures
from PublicationRegistry, which still include the synthetic context parameter
and can shift arguments for legacy routes. Update the route-generation logic in
the REST index code that reads methodRegistry.getSignatures() and
publicationRegistry.getSignatures() so publication signatures are
normalized/stripped before route construction, and ensure the customPublish
wrapper shape does not leak into REST parameter expectations. Adjust the route
handling in the related route-building block as well so publications receive
only their real user args in the correct order.
- Around line 23-35: The legacy REST path is now picking up custom publications
that rely on observer cleanup, but legacyRestPublicationContext provides a no-op
onStop, so those callbacks can leak resources while returning empty results.
Update the REST publication lookup in index.ts to exclude custom publications
(or otherwise filter registry entries so only cursor-returning publications are
used) before invoking them with legacyRestPublicationContext, and keep the no-op
context limited to safe legacy cursor-only handlers.
In `@meteor/server/methodRegistry.ts`:
- Around line 119-135: Prevent method registration after applyToMeteor() has
already snapshotted the registry into Meteor.methods(), since later changes to
MethodRegistry become invisible to Meteor. Add a guard in MethodRegistry.add
(and/or the public registration paths registerApi(), registerMethod(), and
registerDebugMethods()) that throws once applyToMeteor() has been called, so no
new entries can be added to this.methods after the snapshot. Use the existing
MethodRegistry state and identifiers like add and applyToMeteor() to enforce the
freeze consistently across all registration entry points.
In `@meteor/server/publicationRegistry.ts`:
- Around line 79-80: The publication registry is storing and exposing
wrapper/internal parameters instead of the real public API signature. Update
PublicationRegistry.register and customPublish() so extractFunctionSignature is
taken from the typed callback/public-facing function signature, not the wrapper
that includes context, and strip registry-only parameters like context before
saving in this.publications. Ensure introspection/REST returns the original
arguments such as deviceId and token rather than context, ...args.
In `@meteor/server/publications/deviceTriggersPreview.ts`:
- Around line 20-31: The custom publication registration in
registerDeviceTriggersPreviewPublications is losing the original argument
signature because customPublish wraps the callback as context plus rest args,
which breaks PublicationRegistry signature extraction for REST routing. Update
customPublish so it preserves and stores the original callback’s parameter list
from the publication handler passed to
registerDeviceTriggersPreviewPublications, rather than the wrapper signature, so
publicationRegistry.getSignatures() reflects the real route params.
In `@meteor/server/publications/mountedTriggers.ts`:
- Around line 21-24: The mounted-triggers publications are being wrapped in a
way that causes signature introspection to see only the generic async wrapper
instead of the real parameters. Update the registration in mountedTriggers.ts so
the wrapper passed to registry.customPublish preserves the original callback
signature for PeripheralDevicePubSub.mountedTriggersForDevice and the related
publication, keeping parameter names like deviceId, deviceIds, and token visible
to getSignatures() and the REST route/doc generation.
In `@meteor/server/publications/partsUI/publication.ts`:
- Around line 190-218: The `registerPartsUIPublications` callback passed to
`PublicationRegistry.customPublish` is being wrapped in an `async (context,
...args)` handler, which hides the original `playlistId` parameter from
signature inspection. Update `customPublish`/`publishUnsafe` so the original
user callback or its extracted signature is preserved separately for
`getSignatures()`, and keep the `MeteorPubSub.uiParts` handler signature intact
for route generation. Ensure the fix is applied in the
`PublicationRegistry.customPublish` flow rather than changing the
`registerPartsUIPublications` logic itself.
In `@meteor/server/publications/peripheralDevice.ts`:
- Around line 25-80: Validate the subscriber-provided identifiers and authorize
before exposing sensitive fields in peripheralDevice publications. In the
anonymous publication handler and peripheralDevicesAndSubDevices, add runtime
validation for every supplied PeripheralDeviceId/StudioId before using them in
selectors or $in queries, and avoid widening access based solely on token
presence. In peripheralDevice.ts, use the existing access-check helper pattern
from checkAccessAndGetPeripheralDevice or equivalent to confirm the token is
valid before removing secretSettings from peripheralDeviceProjection, and ensure
PeripheralDevicePubSub.peripheralDeviceCommands also validates its deviceId
input before querying.
In `@meteor/server/publications/rundown.ts`:
- Around line 238-249: The Mongo selector in the rundown publications is using
client-supplied args directly, so validate those inputs before assigning them
into query filters. Add runtime check() calls for playlistActivationId and
thisRundownId in the relevant publication handlers in rundown.ts, then only use
the validated scalar values when building the selector in the publication logic
(including the related sections in the same file).
In `@meteor/server/publications/rundownPlaylist.ts`:
- Around line 11-52: The publication callbacks in
registerRundownPlaylistPublications are exposing the internal _context parameter
in the generated signatures, which shifts the legacy REST route shape. Remove
_context from the registry.publish callback signatures for
CorelibPubSub.rundownPlaylists and MeteorPubSub.rundownPlaylistForStudio, and
keep the remaining arguments in the same order so extractFunctionSignature()
produces the expected route segments.
In `@meteor/server/publications/timeline.ts`:
- Around line 40-92: The legacy publication routes in
registerTimelinePublications are exposing transport-only parameters because the
wrapper callback signature is being captured instead of the user-facing DDP
args. Update PublicationRegistry.getSignatures() and the handling used by
createLegacyApiRouter() so publish()/customPublish() entries store only the
actual callback arguments, excluding context and pub, and ensure the timeline
publication registrations in registerTimelinePublications still work with the
corrected signature shape.
---
Nitpick comments:
In `@meteor/server/api/client.ts`:
- Around line 442-449: The exported API method callPeripheralDeviceFunction
still relies on explicit-boundary suppression instead of declaring its return
type like nearby registry methods. Remove the boundary-typing suppression and
add an explicit Promise-based return type on callPeripheralDeviceFunction that
matches its resolved value, keeping the registry surface consistently typed with
the neighboring API methods.
🪄 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: 1c9e766f-c71f-41e9-911b-492aeeff4847
⛔ Files ignored due to path filters (2)
meteor/server/__tests__/__snapshots__/methodRegistry.test.ts.snapis excluded by!**/*.snapmeteor/server/__tests__/__snapshots__/publicationRegistry.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (76)
meteor/__mocks__/helpers/methods.tsmeteor/__mocks__/helpers/publications.tsmeteor/server/__tests__/cronjobs.test.tsmeteor/server/__tests__/methodRegistry.test.tsmeteor/server/__tests__/publicationRegistry.test.tsmeteor/server/api/ExternalMessageQueue.tsmeteor/server/api/__tests__/client.test.tsmeteor/server/api/__tests__/externalMessageQueue.test.tsmeteor/server/api/__tests__/peripheralDevice.test.tsmeteor/server/api/__tests__/rundownLayouts.test.tsmeteor/server/api/__tests__/userActions/general.test.tsmeteor/server/api/__tests__/userActions/system.test.tsmeteor/server/api/blueprints/__tests__/api.test.tsmeteor/server/api/blueprints/api.tsmeteor/server/api/client.tsmeteor/server/api/ingest/debug.tsmeteor/server/api/mongo.tsmeteor/server/api/peripheralDevice.tsmeteor/server/api/playout/api.tsmeteor/server/api/playout/debug.tsmeteor/server/api/rest/api.tsmeteor/server/api/rest/v0/__tests__/rest.test.tsmeteor/server/api/rest/v0/index.tsmeteor/server/api/rundown.tsmeteor/server/api/rundownLayouts.tsmeteor/server/api/showStyles.tsmeteor/server/api/snapshot.tsmeteor/server/api/studio/api.tsmeteor/server/api/system.tsmeteor/server/api/triggeredActions.tsmeteor/server/api/user.tsmeteor/server/api/userActions.tsmeteor/server/lib/customPublication/index.tsmeteor/server/lib/customPublication/publish.tsmeteor/server/main.tsmeteor/server/methodRegistrations.tsmeteor/server/methodRegistry.tsmeteor/server/methods.tsmeteor/server/migration/__tests__/migrations.test.tsmeteor/server/migration/api.tsmeteor/server/publicationRegistrations.tsmeteor/server/publicationRegistry.tsmeteor/server/publications/_publications.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/buckets.tsmeteor/server/publications/deviceTriggersPreview.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/organization.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/packageManager/packageContainers.tsmeteor/server/publications/packageManager/playoutContext.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/peripheralDevice.tsmeteor/server/publications/peripheralDeviceForDevice.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/rundown.tsmeteor/server/publications/rundownPlaylist.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/showStyle.tsmeteor/server/publications/showStyleUI.tsmeteor/server/publications/studio.tsmeteor/server/publications/studioUI.tsmeteor/server/publications/system.tsmeteor/server/publications/timeline.tsmeteor/server/publications/translationsBundles.tsmeteor/server/publications/triggeredActionsUI.tsmeteor/server/security/check.tsmeteor/server/security/securityVerify.tsmeteor/server/systemStatus/__tests__/api.test.tsmeteor/server/systemStatus/__tests__/systemStatus.test.tsmeteor/server/systemStatus/api.ts
💤 Files with no reviewable changes (1)
- meteor/server/publications/_publications.ts
06834fd to
4c6a95f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/publicationRegistry.ts`:
- Around line 90-99: publishUnsafe() still accepts new registrations after
applyToMeteor() has already finalized the registry, which can make
getAllPublicationNames() and verifyAllPublicationsRegistered() see publications
that Meteor.publish() never receives. Update PublicationRegistry so
publishUnsafe() rejects late calls once the registry has been applied (using the
applied state set by applyToMeteor()), and ensure the existing publication
registration flow continues to work only before that point.
🪄 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: 19f25379-83c0-4677-b2a3-7dcadbce54e5
⛔ Files ignored due to path filters (1)
meteor/server/__tests__/__snapshots__/publicationRegistry.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (40)
meteor/__mocks__/helpers/publications.tsmeteor/server/__tests__/publicationRegistry.test.tsmeteor/server/api/rest/api.tsmeteor/server/api/rest/v0/__tests__/rest.test.tsmeteor/server/api/rest/v0/index.tsmeteor/server/lib/customPublication/index.tsmeteor/server/lib/customPublication/publish.tsmeteor/server/main.tsmeteor/server/publicationRegistrations.tsmeteor/server/publicationRegistry.tsmeteor/server/publications/_publications.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/buckets.tsmeteor/server/publications/deviceTriggersPreview.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/organization.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/packageManager/packageContainers.tsmeteor/server/publications/packageManager/playoutContext.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/peripheralDevice.tsmeteor/server/publications/peripheralDeviceForDevice.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/rundown.tsmeteor/server/publications/rundownPlaylist.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/showStyle.tsmeteor/server/publications/showStyleUI.tsmeteor/server/publications/studio.tsmeteor/server/publications/studioUI.tsmeteor/server/publications/system.tsmeteor/server/publications/timeline.tsmeteor/server/publications/translationsBundles.tsmeteor/server/publications/triggeredActionsUI.tsmeteor/server/security/check.ts
💤 Files with no reviewable changes (1)
- meteor/server/publications/_publications.ts
✅ Files skipped from review due to trivial changes (1)
- meteor/server/security/check.ts
🚧 Files skipped from review as they are similar to previous changes (37)
- meteor/server/lib/customPublication/index.ts
- meteor/server/tests/publicationRegistry.test.ts
- meteor/server/publicationRegistrations.ts
- meteor/server/publications/translationsBundles.ts
- meteor/server/publications/rundownPlaylist.ts
- meteor/mocks/helpers/publications.ts
- meteor/server/publications/showStyleUI.ts
- meteor/server/publications/partsUI/publication.ts
- meteor/server/publications/buckets.ts
- meteor/server/publications/deviceTriggersPreview.ts
- meteor/server/publications/blueprintUpgradeStatus/publication.ts
- meteor/server/publications/mountedTriggers.ts
- meteor/server/publications/packageManager/playoutContext.ts
- meteor/server/api/rest/v0/index.ts
- meteor/server/publications/pieceContentStatusUI/rundown/publication.ts
- meteor/server/publications/peripheralDevice.ts
- meteor/server/publications/packageManager/expectedPackages/publication.ts
- meteor/server/publications/ingestStatus/publication.ts
- meteor/server/publications/showStyle.ts
- meteor/server/publications/triggeredActionsUI.ts
- meteor/server/api/rest/v0/tests/rest.test.ts
- meteor/server/publications/partInstancesUI/publication.ts
- meteor/server/publications/lib/lib.ts
- meteor/server/publications/externalEventSubscriptions.ts
- meteor/server/publications/pieceContentStatusUI/bucket/publication.ts
- meteor/server/publications/packageManager/packageContainers.ts
- meteor/server/publications/studioUI.ts
- meteor/server/publications/organization.ts
- meteor/server/publications/segmentPartNotesUI/publication.ts
- meteor/server/publications/timeline.ts
- meteor/server/lib/customPublication/publish.ts
- meteor/server/api/rest/api.ts
- meteor/server/publications/peripheralDeviceForDevice.ts
- meteor/server/publications/rundown.ts
- meteor/server/publications/studio.ts
- meteor/server/main.ts
- meteor/server/publications/system.ts
4c6a95f to
b065834
Compare
About the Contributor
This pull request is posted on behalf of Superfly
Type of Contribution
This is a: Code improvement
This is part of a series of PRs aiming to replace the meteor ddp server.
This builds upon #1783
New Behavior
This continues the pattern started in #1783, expanding it to cover the publications too
Testing
Affected areas
Time Frame
Other Information
Status