Repository navigation
Conversation
Contributor
📝 WalkthroughWalkthroughSe implementa una feature completa de banners: modelo y migración, esquemas OpenAPI, API pública GET /api/banners para banners vigentes, APIs administrativas protegidas (crear/listar/actualizar/toggle) y tests unitarios que cubren validaciones y flujos. ChangesBanners - Public & Admin APIs
Sequence DiagramssequenceDiagram
participant Client
participant BannersController
participant BannersService
participant Prisma
Client->>BannersController: GET /api/banners?limit=10
BannersController->>BannersService: { limit: 10 }
BannersService->>Prisma: findMany(where: status:true, is_active:true, fechas vigentes, take:10)
Prisma-->>BannersService: [banners]
BannersService->>BannersController: banners mapeados
BannersController-->>Client: 200 JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/modules/global/banners/banners.routes.js (1)
13-17: ⚡ Quick winDocumentar límites de
limitpara alinear contrato y validación real.El schema del parámetro debería exponer
minimum,maximumydefaultpara reflejar el comportamiento del servicio y evitar integraciones inválidas.Propuesta
* - in: query * name: limit * schema: * type: integer + * minimum: 1 + * maximum: 50 + * default: 10🤖 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 `@src/modules/global/banners/banners.routes.js` around lines 13 - 17, Actualizar el esquema OpenAPI del parámetro query "limit" en banners.routes.js para que refleje la validación real: en el objeto schema de "limit" añadir minimum (por ejemplo 1), maximum (por ejemplo 100) y default (por ejemplo 20) y asegurarse de que esos valores coincidan con la lógica del endpoint (p. ej. la función/handler que procesa el parámetro "limit" o el middleware de validación usado por la ruta /banners); ajustar los números si la implementación real usa otros límites para mantener el contrato y la validación sincronizados.prisma/schema.prisma (1)
64-76: ⚡ Quick winAgregar
@@indexenBannerspara la consulta de banners activosEl servicio
getActiveBannersServiceconsultastatus=true,is_active=true,start_at <= nowy(end_at IS NULL OR end_at >= now), y ordena porstart_at descyid_banner desc. El modeloBannersno declara@@index, por lo que conviene indexar estos campos.Propuesta
model Banners { @@ updated_at DateTime `@updatedAt` `@db.Timestamptz` + + @@index([status, is_active, start_at]) + @@index([end_at]) }🤖 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 `@prisma/schema.prisma` around lines 64 - 76, The Banners model lacks a compound index for the query used by getActiveBannersService; add an @@index on the fields used for filtering and ordering to speed that query. Edit the model Banners and add a compound index referencing status, is_active, start_at and id_banner (and also include end_at or add a separate @@index on end_at) so the database can optimize the WHERE (status, is_active, start_at, end_at) and ORDER BY (start_at desc, id_banner desc) used by getActiveBannersService; target the model name Banners and the fields status, is_active, start_at, end_at, id_banner when adding the index declarations.tests/unit/admin/admin-banners.test.js (1)
121-154: ⚡ Quick winSumá casos de éxito para
updateAdminBannerServiceytoggleAdminBannerActiveService.Acá sólo se validan errores; falta cubrir el flujo exitoso (incluyendo llamada a
prisma.banners.updatey shape de respuesta), que es clave para el contrato del CRUD admin.Propuesta de tests adicionales
describe("updateAdminBannerService", () => { beforeEach(() => vi.clearAllMocks()); + it("actualiza el banner y devuelve el DTO esperado", async () => { + prisma.banners.findUnique.mockResolvedValue(mockBanner); + prisma.banners.update.mockResolvedValue({ ...mockBanner, title: "Nuevo título" }); + + const result = await updateAdminBannerService(1, { title: " Nuevo título " }); + + expect(prisma.banners.update).toHaveBeenCalled(); + expect(result.title).toBe("Nuevo título"); + }); }); describe("toggleAdminBannerActiveService", () => { beforeEach(() => vi.clearAllMocks()); + it("actualiza isActive cuando el banner existe", async () => { + prisma.banners.findUnique.mockResolvedValue(mockBanner); + prisma.banners.update.mockResolvedValue({ ...mockBanner, is_active: false }); + + const result = await toggleAdminBannerActiveService(1, false); + + expect(prisma.banners.update).toHaveBeenCalledWith( + expect.objectContaining({ data: expect.objectContaining({ is_active: false }) }) + ); + expect(result.isActive).toBe(false); + }); });🤖 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 `@tests/unit/admin/admin-banners.test.js` around lines 121 - 154, Add success-case unit tests for updateAdminBannerService and toggleAdminBannerActiveService: mock prisma.banners.findUnique to return an existing banner (e.g., mockBanner), mock prisma.banners.update to return the updated banner (e.g., mockUpdatedBanner), call updateAdminBannerService(id, payload) and assert it resolves with the expected shape/fields and that prisma.banners.update was called with the correct args; do the same for toggleAdminBannerActiveService(id, isActive) asserting the update call toggles isActive and the returned object shape matches the contract. Ensure tests reference updateAdminBannerService, toggleAdminBannerActiveService, prisma.banners.findUnique and prisma.banners.update, and include assertions for both the update call parameters and the resolved response shape.tests/unit/admin/admin-controllers.test.js (1)
263-297: ⚡ Quick winFalta cubrir el caso de
idinválido en controladores de update/toggle.Conviene agregar tests donde
req.params.idno sea numérico para verificar respuesta 400 y que el servicio no se invoque. Eso protege el contrato de validación del controller.🤖 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 `@tests/unit/admin/admin-controllers.test.js` around lines 263 - 297, Agregar pruebas para el caso de id inválido en los controladores updateAdminBanner y toggleAdminBannerActive: crear nuevos it blocks donde req.params.id sea no numérico (por ejemplo "abc") usando makeCtx, llamar a updateAdminBanner y toggleAdminBannerActive respectivamente, y verificar que res.status haya sido llamado con 400 y que updateAdminBannerService / toggleAdminBannerActiveService no hayan sido invocados; también asegurarse de que next no sea llamado para mantener el contrato de validación del controller.tests/unit/banners/banners.controller.test.js (1)
23-35: ⚡ Quick winEndurecé el contrato de interacción del controller con el servicio.
Además del status, agregá assert de
getActiveBannersServiceconreq.query; y en error, verificánext(err)con la misma instancia. Hoy el test deja pasar wiring incorrecto.🤖 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 `@tests/unit/banners/banners.controller.test.js` around lines 23 - 35, El test debe verificar que el controller invoque correctamente el servicio con los parámetros de consulta y que propague exactamente la misma instancia de error a next; en el caso exitoso, añade un expect(getActiveBannersService).toHaveBeenCalledWith(req.query) (o con el objeto creado por makeCtx) después de llamar a getActiveBanners, y en el caso de fallo crea una instancia de Error (e.g. const err = new Error("fail")), usa getActiveBannersService.mockRejectedValue(err) y verifica expect(next).toHaveBeenCalledWith(err) para asegurar que next recibe la misma instancia; las funciones relevantes son getActiveBanners y getActiveBannersService.tests/unit/banners/banners.service.test.js (1)
26-33: ⚡ Quick winEl assert del filtro temporal es demasiado laxo.
start_at: expect.any(Object)yOR: expect.any(Array)no garantizan que el rango de vigencia esté bien construido. Usá tiempo fijo (vi.useFakeTimers) yarrayContaining/objectContainingpara validar condiciones exactas.Ejemplo de assert más específico
+vi.useFakeTimers(); +vi.setSystemTime(new Date("2026-05-21T00:00:00Z")); await getActiveBannersService({ limit: 5 }); expect(prisma.banners.findMany).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ status: true, is_active: true, - start_at: expect.any(Object), - OR: expect.any(Array), + start_at: expect.objectContaining({ lte: new Date("2026-05-21T00:00:00Z") }), + OR: expect.arrayContaining([ + { end_at: null }, + { end_at: expect.objectContaining({ gte: new Date("2026-05-21T00:00:00Z") }) }, + ]), }), take: 5, }) );🤖 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 `@tests/unit/banners/banners.service.test.js` around lines 26 - 33, El test usa assertions demasiado vagas para el filtro temporal; en la prueba que llama a prisma.banners.findMany fija el tiempo con vi.useFakeTimers (o jest.useFakeTimers) y buildea una fecha fija, luego reemplazá expect.any(Object) para start_at por expect.objectContaining({...}) comprobando las claves exactas usadas (por ejemplo gte/lte con las fechas fijas) y reemplazá OR: expect.any(Array) por expect.objectContaining({ OR: expect.arrayContaining([expect.objectContaining({...}), expect.objectContaining({...})]) }) para validar cada rama del OR de forma explícita; mantiene prisma.banners.findMany como punto de verificación y usa expect.objectContaining/expect.arrayContaining para comparar solo las partes relevantes del filtro.
🤖 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 `@prisma/migrations/20260521004958_add_banners/migration.sql`:
- Around line 2-16: Agregar una constraint CHECK en la definición de la tabla
Banners para evitar que end_at sea anterior a start_at: en la CREATE TABLE
"Banners" (o con un ALTER TABLE posterior) añade una constraint con un nombre
único, por ejemplo Banners_valid_period_chk, que exija (end_at IS NULL OR end_at
>= start_at) sobre las columnas "start_at" y "end_at" para garantizar vigencia
temporal válida.
In `@src/modules/admin/banners/admin-banners.service.js`:
- Around line 160-167: Hay una TOCTOU: el código hace un
prisma.banners.findUnique y comprueba existing.status antes de un update,
permitiendo que otro request cambie status entre ambas operaciones; cambia la
lógica para hacer la actualización condicional/atómica usando
prisma.banners.update (o updateMany) con un where que incluya tanto id_banner:
id como status: true, y luego verifica el resultado (fila afectada o valor
retornado) para lanzar NotFoundError si no se actualizó nada; aplícalo también
en las otras dos ubicaciones que usan el mismo patrón (las llamadas a
prisma.banners.findUnique / update alrededor de bannerSelect en este archivo).
- Around line 133-153: The pagination logic can produce Infinity when limit is
0; before using limit in prisma.banners.findMany (take) and computing
totalPages, normalize it to a safe minimum (e.g., const safeLimit = Math.max(1,
Number(limit) || 1)) and use safeLimit for take and Math.ceil(total /
safeLimit), while still returning the original page/limit inputs if desired;
update references in this block (pagination destructure, prisma.banners.findMany
take, and totalPages calculation) to use the normalized value.
In `@src/modules/global/banners/banners.service.js`:
- Around line 35-37: El guard clause devuelve [] cuando
prisma?.banners?.findMany no existe, ocultando un fallo de
contrato/infraestructura; en src/modules/global/banners/banners.service.js
reemplaza ese return [] por lanzar un error claro (por ejemplo throw new
Error(...)) que incluya contexto sobre el cliente Prisma y la ausencia de
banners.findMany, para que el caller/monitoring detecte el problema; además
actualiza los tests para cubrir el caso donde prisma.banners es undefined y
assertar que se lanza el error.
---
Nitpick comments:
In `@prisma/schema.prisma`:
- Around line 64-76: The Banners model lacks a compound index for the query used
by getActiveBannersService; add an @@index on the fields used for filtering and
ordering to speed that query. Edit the model Banners and add a compound index
referencing status, is_active, start_at and id_banner (and also include end_at
or add a separate @@index on end_at) so the database can optimize the WHERE
(status, is_active, start_at, end_at) and ORDER BY (start_at desc, id_banner
desc) used by getActiveBannersService; target the model name Banners and the
fields status, is_active, start_at, end_at, id_banner when adding the index
declarations.
In `@src/modules/global/banners/banners.routes.js`:
- Around line 13-17: Actualizar el esquema OpenAPI del parámetro query "limit"
en banners.routes.js para que refleje la validación real: en el objeto schema de
"limit" añadir minimum (por ejemplo 1), maximum (por ejemplo 100) y default (por
ejemplo 20) y asegurarse de que esos valores coincidan con la lógica del
endpoint (p. ej. la función/handler que procesa el parámetro "limit" o el
middleware de validación usado por la ruta /banners); ajustar los números si la
implementación real usa otros límites para mantener el contrato y la validación
sincronizados.
In `@tests/unit/admin/admin-banners.test.js`:
- Around line 121-154: Add success-case unit tests for updateAdminBannerService
and toggleAdminBannerActiveService: mock prisma.banners.findUnique to return an
existing banner (e.g., mockBanner), mock prisma.banners.update to return the
updated banner (e.g., mockUpdatedBanner), call updateAdminBannerService(id,
payload) and assert it resolves with the expected shape/fields and that
prisma.banners.update was called with the correct args; do the same for
toggleAdminBannerActiveService(id, isActive) asserting the update call toggles
isActive and the returned object shape matches the contract. Ensure tests
reference updateAdminBannerService, toggleAdminBannerActiveService,
prisma.banners.findUnique and prisma.banners.update, and include assertions for
both the update call parameters and the resolved response shape.
In `@tests/unit/admin/admin-controllers.test.js`:
- Around line 263-297: Agregar pruebas para el caso de id inválido en los
controladores updateAdminBanner y toggleAdminBannerActive: crear nuevos it
blocks donde req.params.id sea no numérico (por ejemplo "abc") usando makeCtx,
llamar a updateAdminBanner y toggleAdminBannerActive respectivamente, y
verificar que res.status haya sido llamado con 400 y que
updateAdminBannerService / toggleAdminBannerActiveService no hayan sido
invocados; también asegurarse de que next no sea llamado para mantener el
contrato de validación del controller.
In `@tests/unit/banners/banners.controller.test.js`:
- Around line 23-35: El test debe verificar que el controller invoque
correctamente el servicio con los parámetros de consulta y que propague
exactamente la misma instancia de error a next; en el caso exitoso, añade un
expect(getActiveBannersService).toHaveBeenCalledWith(req.query) (o con el objeto
creado por makeCtx) después de llamar a getActiveBanners, y en el caso de fallo
crea una instancia de Error (e.g. const err = new Error("fail")), usa
getActiveBannersService.mockRejectedValue(err) y verifica
expect(next).toHaveBeenCalledWith(err) para asegurar que next recibe la misma
instancia; las funciones relevantes son getActiveBanners y
getActiveBannersService.
In `@tests/unit/banners/banners.service.test.js`:
- Around line 26-33: El test usa assertions demasiado vagas para el filtro
temporal; en la prueba que llama a prisma.banners.findMany fija el tiempo con
vi.useFakeTimers (o jest.useFakeTimers) y buildea una fecha fija, luego
reemplazá expect.any(Object) para start_at por expect.objectContaining({...})
comprobando las claves exactas usadas (por ejemplo gte/lte con las fechas fijas)
y reemplazá OR: expect.any(Array) por expect.objectContaining({ OR:
expect.arrayContaining([expect.objectContaining({...}),
expect.objectContaining({...})]) }) para validar cada rama del OR de forma
explícita; mantiene prisma.banners.findMany como punto de verificación y usa
expect.objectContaining/expect.arrayContaining para comparar solo las partes
relevantes del filtro.
🪄 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: 741000eb-ec0a-4427-9128-2a5162ba88c1
📒 Files selected for processing (18)
prisma/migrations/20260521004958_add_banners/migration.sqlprisma/schema.prismasrc/app.jssrc/config/swagger.config.jssrc/docs/schemas/admin/admin-banner.schema.jssrc/docs/schemas/banner.schema.jssrc/docs/schemas/index.jssrc/modules/admin/banners/admin-banners.controller.jssrc/modules/admin/banners/admin-banners.routes.jssrc/modules/admin/banners/admin-banners.service.jssrc/modules/admin/index.jssrc/modules/global/banners/banners.controller.jssrc/modules/global/banners/banners.routes.jssrc/modules/global/banners/banners.service.jstests/unit/admin/admin-banners.test.jstests/unit/admin/admin-controllers.test.jstests/unit/banners/banners.controller.test.jstests/unit/banners/banners.service.test.js
|
Contributor
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 (2)
src/modules/admin/banners/admin-banners.service.js (2)
162-188:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLa actualización parcial todavía puede pisar cambios concurrentes.
startAt/endAtse validan contraexisting, pero el write sólo condiciona porid_bannerystatus. Si dos admins editan fechas en paralelo, ambos requests pueden pasar la validación y terminar guardando un rango inválido o sobrescribiendo el cambio del otro. Acá necesitás merge atómico: transacción con lock o optimistic locking conupdated_aten elwhere.Also applies to: 216-226
🤖 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 `@src/modules/admin/banners/admin-banners.service.js` around lines 162 - 188, The patch validates startAt/endAt against the fetched existing record but then updates without protecting against concurrent edits, so change the update to perform an atomic merge using optimistic locking: include the record's current timestamp (existing.updated_at) in the update WHERE clause (or run the read+write inside a DB transaction with a row lock) so the UPDATE on prisma.banners only succeeds if updated_at matches the value you validated; if the update affects 0 rows, throw a concurrency error and surface that to the caller. Apply the same pattern for the other update block referenced around the 216-226 region so both partial-update flows use the updated_at-based WHERE (or transaction+lock) to prevent lost updates.
133-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
safeLimit/safeSkiptodavía permiten valores inválidos; normalizalos a enteros finitosLos guards actuales aceptan
2.5yInfinity, que terminan entake/skipy también entotalPages. Para Prisma,skip/takerepresentan cantidades/offsets de paginación y se deben usar como enteros (evitar decimales yInfinity).💡 Ajuste mínimo
const { page = 1, limit = 20, skip = 0 } = pagination; - const safeLimit = Number(limit) > 0 ? Number(limit) : 20; - const safeSkip = Number(skip) >= 0 ? Number(skip) : 0; + const parsedLimit = Number(limit); + const parsedSkip = Number(skip); + const safeLimit = Number.isInteger(parsedLimit) && parsedLimit > 0 ? parsedLimit : 20; + const safeSkip = Number.isInteger(parsedSkip) && parsedSkip >= 0 ? parsedSkip : 0;🤖 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 `@src/modules/admin/banners/admin-banners.service.js` around lines 133 - 154, The current safeLimit/safeSkip allow non-integer and non-finite values (e.g., 2.5 or Infinity); update the normalization where safeLimit and safeSkip are computed so they coerce to finite integers: use Number.isFinite on Number(limit)/Number(skip) and apply Math.floor (or parseInt) then clamp safeLimit to a minimum of 1 and safeSkip to a minimum of 0; ensure the updated safeLimit/safeSkip are used in prisma.banners.findMany (take/skip) and in the totalPages calculation (Math.ceil(total / safeLimit)).
🤖 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.
Outside diff comments:
In `@src/modules/admin/banners/admin-banners.service.js`:
- Around line 162-188: The patch validates startAt/endAt against the fetched
existing record but then updates without protecting against concurrent edits, so
change the update to perform an atomic merge using optimistic locking: include
the record's current timestamp (existing.updated_at) in the update WHERE clause
(or run the read+write inside a DB transaction with a row lock) so the UPDATE on
prisma.banners only succeeds if updated_at matches the value you validated; if
the update affects 0 rows, throw a concurrency error and surface that to the
caller. Apply the same pattern for the other update block referenced around the
216-226 region so both partial-update flows use the updated_at-based WHERE (or
transaction+lock) to prevent lost updates.
- Around line 133-154: The current safeLimit/safeSkip allow non-integer and
non-finite values (e.g., 2.5 or Infinity); update the normalization where
safeLimit and safeSkip are computed so they coerce to finite integers: use
Number.isFinite on Number(limit)/Number(skip) and apply Math.floor (or parseInt)
then clamp safeLimit to a minimum of 1 and safeSkip to a minimum of 0; ensure
the updated safeLimit/safeSkip are used in prisma.banners.findMany (take/skip)
and in the totalPages calculation (Math.ceil(total / safeLimit)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cae52487-fbd0-4350-aa64-e23a238499af
📒 Files selected for processing (2)
src/modules/admin/banners/admin-banners.service.jssrc/modules/global/banners/banners.service.js
leoAchu16
approved these changes
May 21, 2026
This was referenced Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Notas de la Versión
New Features
Documentation
Tests