feat: implement multi-banner system with role/group targeting#12697
feat: implement multi-banner system with role/group targeting#12697Airamhh wants to merge 1 commit intodanny-avila:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements a multi-banner system with role/group/user targeting, scheduling, and an admin CRUD API surface, plus a new client-side banner carousel.
Changes:
- Extended the Banner schema/model and added banner CRUD/query methods (including multi-audience filtering).
- Added new API routes for listing active banners and for admin banner management with capability checks.
- Updated the client to render multiple banners via a rotating carousel and added supporting hooks/queries.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/schema/banner.ts | Adds optional multi-banner targeting fields and several indexes. |
| packages/data-schemas/src/methods/banner.ts | Implements multi-banner querying + CRUD, audience validation, and admin helpers. |
| packages/data-schemas/src/methods/banner.spec.ts | Adds unit tests for the new banner methods. |
| packages/data-schemas/src/admin/capabilities.ts | Introduces READ_BANNERS / MANAGE_BANNERS capabilities and implication. |
| packages/data-provider/src/schemas.ts | Updates banner schema/types for new fields and optional displayTo. |
| packages/data-provider/src/keys.ts | Adds react-query keys for banners/admin banners. |
| packages/data-provider/src/data-service.ts | Adds data-service functions for multi-banner + admin banner CRUD. |
| packages/data-provider/src/api-endpoints.ts | Adds /api/banner/list and admin banner endpoint builders. |
| packages/api/src/admin/index.ts | Exports the new admin banners handlers. |
| packages/api/src/admin/banners.ts | Adds admin banner CRUD handlers (create/list/get/update/delete/toggle). |
| config/migrate-banners.js | Adds a migration script to backfill defaults for new banner fields. |
| client/src/routes/Root.tsx | Switches from legacy single Banner to the new BannerCarousel. |
| client/src/locales/en/translation.json | Adds i18n strings for carousel navigation/dismiss actions. |
| client/src/hooks/Banners/useBannerRotation.ts | Adds rotation/controls hook for banner carousel behavior. |
| client/src/hooks/Banners/index.ts | Exports the new banner rotation hook. |
| client/src/data-provider/index.ts | Exports new banners query hooks. |
| client/src/data-provider/Banners/useBannersQuery.ts | Adds query hook to fetch active banners list. |
| client/src/data-provider/Banners/useBannerQuery.ts | Adds legacy “first banner” query hook. |
| client/src/data-provider/Banners/index.ts | Barrel export for banner query hooks. |
| client/src/components/Banners/index.ts | Exports BannerCarousel alongside legacy Banner. |
| client/src/components/Banners/BannerCarousel.tsx | New carousel UI with sanitization, rotation, and dismissal behavior. |
| client/src/components/Banners/Banner.tsx | Adds DOMPurify sanitization to legacy banner rendering. |
| api/server/routes/index.js | Registers the new admin banners router. |
| api/server/routes/banner.js | Adds /api/banner/list endpoint while keeping legacy /api/banner. |
| api/server/routes/admin/banners.js | New admin banners router wired with capability middleware. |
| api/server/index.js | Mounts /api/admin/banners route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| audienceMode?: 'global' | 'role' | 'group' | 'user'; | ||
| targetRoleIds?: string[]; | ||
| targetGroupIds?: string[]; | ||
| targetUserIds?: string[]; |
There was a problem hiding this comment.
targetRoleIds is treated as role names (e.g., user.role) in getActiveBanners() and validated via Role.find({ name: { $in: roleIds } }), so calling these fields *Ids is misleading and can cause incorrect usage (e.g., passing Role ObjectIds). Consider renaming to targetRoleNames (and corresponding audience query/validation), or change the implementation to actually store and query role ObjectIds consistently.
| $or: [{ displayTo: { $gte: now } }, { displayTo: null }], | ||
| $and: [ | ||
| { | ||
| $or: [{ displayTo: { $gte: now } }, { displayTo: null }], |
There was a problem hiding this comment.
The active-window filter only matches displayTo >= now or displayTo: null. Documents where displayTo is missing/undefined (common if it’s omitted on create) won’t match and will never be returned. Include { displayTo: { $exists: false } } in the $or, or persist displayTo as null when omitted.
| $or: [{ displayTo: { $gte: now } }, { displayTo: null }], | |
| $or: [ | |
| { displayTo: { $gte: now } }, | |
| { displayTo: null }, | |
| { displayTo: { $exists: false } }, | |
| ], |
| getBannerById: (id: string) => Promise<unknown | null>; | ||
| updateBanner: ( | ||
| id: string, | ||
| updates: unknown, | ||
| bannerId?: undefined, | ||
| tenantId?: string, | ||
| ) => Promise<unknown | null>; |
There was a problem hiding this comment.
AdminBannersDeps.updateBanner/deleteBanner/toggleBanner include placeholder params like bannerId?: undefined, which obscures the contract and doesn’t match the underlying banner methods (they accept session?: ClientSession + tenantId?: string). Update these signatures to reflect the real parameters so the handler contract stays type-safe and understandable.
| const requireReadBanners = requireCapability(SystemCapabilities.READ_BANNERS); | ||
| const requireManageBanners = requireCapability(SystemCapabilities.MANAGE_BANNERS); | ||
|
|
||
| // TODO: Still missing the capabilities integration, need to talk this out with Danny |
There was a problem hiding this comment.
This TODO is misleading: capability middleware is already wired up below (requireReadBanners/requireManageBanners). If the intent is “admin panel UI integration is missing” (or similar), please reword to reflect the actual remaining work so it doesn’t read like backend access control is incomplete.
| // TODO: Still missing the capabilities integration, need to talk this out with Danny | |
| // TODO: Backend capability enforcement is already wired up below; confirm what admin UI/capability exposure work still remains with Danny. |
| export function getAdminBanners(params: AdminBannersParams = {}): Promise<AdminBannersResult> { | ||
| const queryParams = new URLSearchParams(); | ||
| if (params.page) { | ||
| queryParams.append('page', params.page.toString()); | ||
| } |
There was a problem hiding this comment.
Pagination params don’t match the server: this endpoint sends page, but the server pagination helper reads offset (and derives page from it). As-is, the server will default offset to 0 and effectively always return page 1. Consider sending offset = (page - 1) * limit (or updating the server to accept page).
| ...data, | ||
| bannerId, | ||
| message: data.message.trim(), | ||
| displayFrom: data.displayFrom || new Date(), |
There was a problem hiding this comment.
createBanner() doesn’t set displayTo when it’s omitted. With the current query logic in getActiveBanners(), an omitted end date should be saved as null (or the query updated), otherwise the banner may never be considered active.
| displayFrom: data.displayFrom || new Date(), | |
| displayFrom: data.displayFrom || new Date(), | |
| displayTo: data.displayTo ?? null, |
| return null; | ||
| } | ||
|
|
||
| banner.isActive = !banner.isActive; |
There was a problem hiding this comment.
toggleBanner uses banner.isActive = !banner.isActive;. If isActive is undefined on legacy documents, this sets it to true (no-op) instead of toggling from the effective default (true) to false. Consider treating undefined as true when toggling (e.g., banner.isActive = !(banner.isActive ?? true)).
| banner.isActive = !banner.isActive; | |
| banner.isActive = !(banner.isActive ?? true); |
| mockUser = { | ||
| _id: new Types.ObjectId('507f1f77bcf86cd799439011'), | ||
| username: 'testuser', | ||
| role: 'USER', | ||
| }; |
There was a problem hiding this comment.
beforeEach assigns to mockUser = { ... }, but the file only declares let _mockUser; (and never declares mockUser). This will throw at runtime / fail linting. Declare let mockUser; (or rename _mockUser -> mockUser) and use it consistently.
| message: z.string(), | ||
| displayFrom: z.string(), | ||
| displayTo: z.string(), | ||
| displayTo: z.string().optional(), |
There was a problem hiding this comment.
displayTo can be null in existing data (e.g., config/update-banner.js sets displayTo = null when not specified). Changing this to z.string().optional() still doesn’t model null, so downstream TS types remain inaccurate. Consider z.string().nullable().optional() (or a union) to reflect actual API responses.
| displayTo: z.string().optional(), | |
| displayTo: z.string().nullable().optional(), |
| const { limit, offset } = parsePagination(req.query); | ||
| const page = Math.floor(offset / limit) + 1; | ||
| const { audienceMode, isActive } = req.query; |
There was a problem hiding this comment.
parsePagination reads limit + offset, but this handler computes page from offset and ignores any page query param. The corresponding client currently sends page, so pagination will be stuck on page 1 unless the API is updated to accept page or the client is updated to send offset.
3474c06 to
de975df
Compare
Implements a comprehensive multi-banner system with advanced audience targeting and management capabilities. Backend: - Add banner schema with 23 fields including audience targeting (role, group, user) - Create banner methods in data-schemas (CRUD + getActiveBanners, toggleBanner, listBanners) - Add admin API handlers in packages/api/src/admin/banners.ts - Add admin routes in api/server/routes/admin/banners.js - Add READ_BANNERS and MANAGE_BANNERS system capabilities - Add migration script config/migrate-banners.js - Follow LibreChat conventions: * CreateBannerRequest/UpdateBannerRequest in types/banner.ts (like groups.ts, roles.ts) * Import types from @librechat/data-schemas (following user.ts pattern) * No 'unknown' or 'any' types - all properly typed with IBanner interfaces * Capability-based access control matching groups/roles patterns Frontend: - Create BannerCarousel component with auto-rotation - Add useBannerRotation hook for automatic banner cycling - Add React Query integration (useBannerQuery, useBannersQuery) - Add 29 localization keys in en-US - Render banners in Root.tsx Data Provider: - Add banner API endpoints and schemas - Add banner query keys - Add data service functions Testing: - Add comprehensive unit tests (25 tests, 56% coverage) - Tests use MongoMemoryServer following agent.spec.ts pattern - All tests passing with 0 TypeScript errors, 0 lint errors Features: - Global, role-based, group-based, and user-specific targeting - Scheduled banners (displayFrom/displayTo date range) - Priority-based ordering - Active/inactive toggle - Persistent/dismissable banners - Tenant isolation support
de975df to
423ff8f
Compare
|
Yaaasss, finally passing the tests 😄 Since I know Copilot can keep me on and endless loop... I think the best thing would be if you can review it whenever you can and I can work on any potential change needed |
Summary
Greetings!
This PR implements a multi-banner system with role/group targeting capabilities for LibreChat
The implementation follows LibreChat's established patterns for admin features, using capability-based access control and the handler abstraction layer
Closes #12696
Features:
PR ToDo
Change Type
Please delete any irrelevant options.
Testing
I had an script that created multiple banners for roles and groups to test them since I didn't want to touch a lot the script (since the idea would be create it into the admin panel) but the update banner script could be improved
Checklist
Please delete any irrelevant options.