chore: migrate instances.get endpoint to new chained API pattern#38881
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: e393949 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughRefactors the instances API route from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-instances-api-chained-pattern.mdapps/meteor/app/api/server/v1/instances.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/instances.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/refactor-instances-api-chained-pattern.md (1)
1-5: LGTM!Changeset is correctly formatted. The
patchbump is appropriate for this internal refactor.apps/meteor/app/api/server/v1/instances.ts (3)
2-6: LGTM on imports.All imported symbols (
ajv,validateUnauthorizedErrorResponse,validateForbiddenErrorResponse) are used in the route configuration below.
73-96: Handler logic cleanly preserved.The data-fetching, mapping, and response construction are straightforward and correctly migrated. The optional chaining on
connectiongracefully handles missing connections.
20-71: Route configuration and schema structure look good.Auth/permission guards are correctly preserved, error response mappings are properly wired, and the envelope validation (
success: true,additionalProperties: false) is tight. The compile-at-module-load pattern is appropriate for AJV.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/instances.ts`:
- Around line 55-58: The response schema's required array omits instanceRecord
even though the handler and the TypeScript response type include it; update the
schema's required array (the one currently set to ['currentStatus',
'broadcastAuth']) to also include 'instanceRecord' so it becomes
['currentStatus', 'broadcastAuth', 'instanceRecord'], ensuring the schema
matches the TypeScript type and the handler output (references: instanceRecord,
currentStatus, broadcastAuth, required).
smirk-dev
force-pushed
the
refactor/migrate-instances-api-chained-pattern
branch
from
6d1e6dc to
7df784b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-instances-api-chained-pattern.mdapps/meteor/app/api/server/v1/instances.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/instances.ts
🧠 Learnings (1)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/instances.ts (1)
69-92: LGTM!The
action()handler correctly awaits both async calls, preserves the original data-aggregation logic, and usesAPI.v1.successas expected by the chained pattern..changeset/refactor-instances-api-chained-pattern.md (1)
1-5: LGTM!Correct package name, appropriate
patchbump, and description aligns with the migration objective.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/instances.ts`:
- Line 24: The JSON schema marks address, lastHeartbeatTime, and local as
nullable but the TypeScript generics use optional (e.g., address?: string;
lastHeartbeatTime?: number; local?: boolean;) and the handler never emits null;
remove the nullable: true from those property definitions in the JSON/OpenAPI
schema(s) that describe the Instances/response object so the schema matches the
TypeScript types (leave the TypeScript optional properties unchanged). Ensure
you update every schema occurrence referencing those properties (the schema
objects that define address, lastHeartbeatTime, and local).
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/instances.ts`:
- Around line 51-54: The JSON schema's required array is missing the
instanceRecord property—update the schema so required includes 'instanceRecord'
alongside 'currentStatus' and 'broadcastAuth' to match the TypeScript types and
the handler that always sets instanceRecord; locate the schema object that
defines properties { instanceRecord, broadcastAuth, currentStatus } and add
'instanceRecord' to its required array to keep the contract accurate.
Migrates the instances.get REST API endpoint from the legacy API.v1.addRoute() pattern to the new chained .get() pattern with: - Typed response schemas for OpenAPI spec generation - Standardized error response validators (401, 403) - Named action function instead of anonymous method Part of RocketChat#38876
smirk-dev
force-pushed
the
refactor/migrate-instances-api-chained-pattern
branch
from
7df784b to
deef73c
Compare
There was a problem hiding this comment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/refactor-instances-api-chained-pattern.mdapps/meteor/app/api/server/v1/instances.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/refactor-instances-api-chained-pattern.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/instances.ts
🧠 Learnings (3)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/instances.ts (2)
69-92: LGTM!The migrated action logic faithfully preserves the existing behavior: unbounded
InstanceStatus.find()fetch,getConnections()lookup, per-instance mapping with safe optional chaining, and hardcodedbroadcastAuth: true— all consistent with the PR's stated goal of a behavioral no-op refactor.
22-62: This is the established convention across the codebase —successshould be omitted from the generic type.The pattern of excluding
successfrom theajv.compile<T>generic while including it in the AJV schema is consistent across all API endpoints in this codebase. Examples:chat.ts,webdav.ts,users.ts,permissions.ts,oauthapps.tsall follow the same pattern. This indicates thatsuccessis automatically appended by the framework (viaAPI.v1.success()), so the generic type should only describe the custom payload. The code is correct as-is.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/instances.ts`:
- Line 54: The JSON schema's required array (currently required:
['currentStatus', 'broadcastAuth']) is missing "instanceRecord" even though the
handler sets instanceRecord and the TypeScript generic marks it non-optional;
update that required array to include 'instanceRecord' so the AJV/OpenAPI schema
matches the TypeScript types and the handler (look for the schema object where
required: ['currentStatus','broadcastAuth'] in
apps/meteor/app/api/server/v1/instances.ts and the handler that sets
instanceRecord).
- Around line 41-47: The schema incorrectly marks fields that are omitted
(undefined) as nullable; update the schema where the response defines address
and currentStatus.properties to remove nullable: true from address,
lastHeartbeatTime, and local and instead treat them as optional/omitted
properties (i.e., leave them out of the OpenAPI nullable list so the spec
reflects that they are absent when missing); locate the schema definition
referencing address and currentStatus (the properties named address,
lastHeartbeatTime, and local) and delete the nullable: true entries for those
three fields.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38881 +/- ##
===========================================
- Coverage 70.64% 70.60% -0.05%
===========================================
Files 3190 3190
Lines 112732 112732
Branches 20421 20406 -15
===========================================
- Hits 79642 79596 -46
- Misses 31047 31087 +40
- Partials 2043 2049 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- Add instanceRecord to required array in schema - Remove nullable: true from optional fields (address, lastHeartbeatTime, local) - Add success: true to TypeScript generic type
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/api/server/v1/instances.ts (1)
2-68:⚠️ Potential issue | 🟡 MinorFix response structure to handle missing connections properly.
When no matching connection is found,
connection?.lastHeartbeatTimebecomesundefinedin the response object. The AJV schema enforces{ type: 'number' }for this field, which rejectsundefinedvalues even though the field is not in the required array. Either omitlastHeartbeatTimewhen the connection doesn't exist or restructure to avoid includingundefinedproperties in the response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/instances.ts` around lines 2 - 68, The response builder for the API.v1.get handler 'instances.get' is including currentStatus.lastHeartbeatTime with an undefined value when no connection exists, which violates the AJV schema (type: 'number'); update the code that assembles each instance's currentStatus (where you reference connection?.lastHeartbeatTime) to only include the lastHeartbeatTime property when it is a defined number (e.g., conditionally add the property or use a spread with connection && { lastHeartbeatTime: connection.lastHeartbeatTime }), so the response omits the property entirely when no connection is found and remains a number when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/api/server/v1/instances.ts`:
- Around line 2-68: The response builder for the API.v1.get handler
'instances.get' is including currentStatus.lastHeartbeatTime with an undefined
value when no connection exists, which violates the AJV schema (type: 'number');
update the code that assembles each instance's currentStatus (where you
reference connection?.lastHeartbeatTime) to only include the lastHeartbeatTime
property when it is a defined number (e.g., conditionally add the property or
use a spread with connection && { lastHeartbeatTime:
connection.lastHeartbeatTime }), so the response omits the property entirely
when no connection is found and remains a number when present.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/instances.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/instances.ts
🧠 Learnings (4)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/instances.ts
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/instances.ts (1)
70-92: LGTM: handler preserves behavior and aligns with new route shape.Mapping instances from the records/connections and returning via
API.v1.successmatches the expected response contract.
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
ggazzo
changed the title
|
/jira ARCH-1935 |
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
github-project-automation
bot
moved this from Backlog
to Done
in rest api conversion to new endpoints
3 participants
Summary
Migrates the
instances.getGET endpoint from the legacyAPI.v1.addRoute()pattern to the new chained.get()API pattern.Changes
addRoute('instances.get', ..., { async get() {} })withAPI.v1.get('instances.get', { ... }, async function action() {})instancesarray withaddress,currentStatus,instanceRecord, andbroadcastAuthfieldsMotivation
Part of the ongoing effort to migrate REST API endpoints to the new chained API pattern that enables typed response validation and future OpenAPI spec generation.
Closes part of #38876
COMM-144
Summary by CodeRabbit
Refactor
Chores
Task: ARCH-2004