Skip to content

refactor(api): migrate banners GET endpoint to OpenAPI with AJV validation#39404

Closed
amitb0ra wants to merge 4 commits intoRocketChat:developfrom
amitb0ra:refactor/banners-openapi-migration
Closed

refactor(api): migrate banners GET endpoint to OpenAPI with AJV validation#39404
amitb0ra wants to merge 4 commits intoRocketChat:developfrom
amitb0ra:refactor/banners-openapi-migration

Conversation

@amitb0ra
Copy link
Copy Markdown
Contributor

@amitb0ra amitb0ra commented Mar 6, 2026

Proposed changes

Migrate the banners GET endpoint to the new OpenAPI-compatible route definition syntax with AJV query and response validation.

Changes

apps/meteor/app/api/server/v1/banners.ts

  • Migrated banners from addRoute() to API.v1.get() with async function action() handler
  • Added AJV query schema validating platform as enum: ['web', 'mobile']
  • Added response validation schemas (200, 400, 401)
  • Used type: 'object' for IBanner[] items (IBanner is not registered in typia due to z.custom<UiKit.BannerView> dependency)
  • Added ExtractRoutesFromAPI and declare module augmentation for automatic type extraction
  • Preserved existing business logic unchanged

packages/rest-typings/src/v1/banners.ts

  • Removed Banners type and /v1/banners endpoint entry from BannersEndpoints
  • Consolidated isBannersProps to compile against BannersId (shared schema, same shape)
  • Kept BannersId, BannersDismiss, and their validators (still used by non-migrated endpoints)

apps/meteor/tests/end-to-end/api/banners.ts

  • Updated error type assertions for [/banners] tests from 'invalid-params' to 'error-invalid-params' to match new typed route validation behavior

.changeset/migrate-banners-openapi.md

  • Added changeset with minor bump for @rocket.chat/meteor and @rocket.chat/rest-typings

Issue(s)

Tracking: RocketChat/Rocket.Chat-Open-API#150

Test results

image

Summary by CodeRabbit

  • Improvements
    • Banners GET now uses stricter request validation, typed responses, and standardized error handling for invalid parameters and auth failures.
  • Bug Fixes
    • Updated tests to expect the new validation error codes.
  • Documentation
    • Added a changeset documenting the migration of the banners endpoint to the OpenAPI-style route.
  • Public API
    • Typing/schema updates to include banner types and align endpoint typings.

@amitb0ra amitb0ra requested review from a team as code owners March 6, 2026 05:05
@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot bot commented Mar 6, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 6, 2026

🦋 Changeset detected

Latest commit: 11a018d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/rest-typings Minor
@rocket.chat/meteor Minor
@rocket.chat/api-client Patch
@rocket.chat/core-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/http-router Patch
@rocket.chat/models Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/livechat Patch
@rocket.chat/mock-providers Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/server-fetch Patch
@rocket.chat/ui-client Major
@rocket.chat/media-calls Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/ui-voip Major
@rocket.chat/core-typings Minor
@rocket.chat/apps Patch
@rocket.chat/model-typings Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e4351457-5774-4804-b01c-0bd578d8627d

📥 Commits

Reviewing files that changed from the base of the PR and between 08eb9de and 1de1879.

📒 Files selected for processing (2)
  • apps/meteor/app/api/server/v1/banners.ts
  • packages/core-typings/src/Ajv.ts
📜 Recent 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). (1)
  • GitHub Check: cubic · AI code reviewer
🧰 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/banners.ts
  • packages/core-typings/src/Ajv.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.

Applied to files:

  • apps/meteor/app/api/server/v1/banners.ts
📚 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/banners.ts
  • packages/core-typings/src/Ajv.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.

Applied to files:

  • apps/meteor/app/api/server/v1/banners.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation

Applied to files:

  • apps/meteor/app/api/server/v1/banners.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/banners.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Applied to files:

  • apps/meteor/app/api/server/v1/banners.ts
  • packages/core-typings/src/Ajv.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

Applied to files:

  • apps/meteor/app/api/server/v1/banners.ts
  • packages/core-typings/src/Ajv.ts
🔇 Additional comments (5)
packages/core-typings/src/Ajv.ts (1)

4-4: LGTM! IBanner correctly added to typia schema generation.

The import is properly ordered and the type is added to the union to enable $ref: '#/components/schemas/IBanner' usage in the banners endpoint response schema.

Also applies to: 17-17

apps/meteor/app/api/server/v1/banners.ts (4)

1-12: LGTM! Imports are correctly structured.

All new imports are necessary for the OpenAPI migration: BannerPlatform and IBanner for type annotations, ajv for inline schema compilation, error validators for standardized responses, and ExtractRoutesFromAPI for type extraction.


108-114: LGTM! Handler correctly implements the endpoint logic.

The handler properly extracts the validated platform parameter and delegates to Banner.getBannersForUser without the id parameter (correctly distinct from the /banners/:id endpoint). Business logic is preserved unchanged.


163-168: LGTM! Module augmentation follows established pattern.

The type extraction and module augmentation correctly extends the Endpoints interface with the new route type. The eslint-disable comments are necessary for the augmentation pattern.

Note: Per project architecture, this augmentation in apps/meteor may not be visible to packages like ddp-client when compiled in isolation. Based on learnings from PR #38913, this is expected behavior and such consumers may need to handle this case separately.


91-103: Verify if response validation actually uses IBanner schema at runtime.

The $ref: '#/components/schemas/IBanner' reference in the response schema (line 97) references a schema that is never registered with AJV using addSchema(). Additionally, the view field in IBannerSchema uses z.custom<UiKit.BannerView>() without an explicit schema provider, which could result in validation being overly permissive or failing silently.

However, E2E tests pass without errors, suggesting either response validation is not enforced at runtime or AJV handles this gracefully. Confirm whether response schemas are actively validated, and if so, verify the view field properly validates UiKit.BannerView structure.


Walkthrough

Migrates the banners GET endpoint to an OpenAPI-compatible route: adds AJV validation for query parameters, defines explicit response schemas and standardized error responses, augments rest-typings with the new endpoint types, and updates tests to reflect changed validation error types.

Changes

Cohort / File(s) Summary
OpenAPI Banners Migration
apps/meteor/app/api/server/v1/banners.ts
Refactors the GET /v1/banners route into an OpenAPI-style bannersEndpoints definition with AJV-driven query validation (platform enum), explicit 200/400/401 response schemas, auth requirement, and uses typed route extraction.
Type & Schema Updates
packages/rest-typings/src/v1/banners.ts, packages/core-typings/src/Ajv.ts
Adjusts isBannersProps AJV compile type from Banners to BannersId; removes /v1/banners GET from previous BannersEndpoints export; adds IBanner to exported Ajv schemas union.
Tests & Changeset
apps/meteor/tests/end-to-end/api/banners.ts, .changeset/migrate-banners-openapi.md
Updates tests to expect errorType error-invalid-params (was invalid-params) for specific validation failures; adds a changeset documenting the migration and version bumps.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

type: chore

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: migrating the banners GET endpoint to OpenAPI with AJV validation, which aligns with the core modifications across all changed files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as this one #39353

@amitb0ra amitb0ra closed this Mar 12, 2026
@amitb0ra
Copy link
Copy Markdown
Contributor Author

closing as handled in #39265

@amitb0ra amitb0ra deleted the refactor/banners-openapi-migration branch March 12, 2026 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants