Conversation
📝 WalkthroughWalkthroughAdds a filtering-capable entity schema to import-export: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/schemas/common/import-export.js (1)
12-24:filtersEntitySchemaobject branch accepts an empty{}— consider addingminProperties: 1.The object branch has no
requiredfields and nominProperties, so"filters": {}would pass validation despite having no semantic meaning. If the intent is that an object-form filter must specify at leastincludeorexclude, add a constraint.Additionally,
includeandexcludearrays lackminItems: 1, unlikeentityProp(Line 8). An entity with"filters": { "include": [] }would validate but is likely not meaningful.♻️ Proposed fix
const filtersEntitySchema = { oneOf: [ { const: false }, { type: 'object', properties: { - include: { type: 'array', items: stringType }, - exclude: { type: 'array', items: stringType } + include: { type: 'array', items: stringType, minItems: 1 }, + exclude: { type: 'array', items: stringType, minItems: 1 } }, - additionalProperties: false + additionalProperties: false, + minProperties: 1 } ] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/schemas/common/import-export.js` around lines 12 - 24, filtersEntitySchema's object branch currently allows an empty object and empty include/exclude arrays; update the object branch in filtersEntitySchema to add "minProperties: 1" so an object must contain at least one property, and add "minItems: 1" to both the include and exclude array schemas (matching the entityProp pattern) so empty arrays are rejected; change these constraints on the filtersEntitySchema definition (referencing filtersEntitySchema, include, and exclude) to enforce meaningful filter content.tests/mocks/schemas/browse.json (1)
14-24: Consider adding a mock entity with bothincludeandexcludeinfilters.The PR description mentions validating entities with filters containing both keys, but neither this file nor
browse-countDown.jsonincludes an entity with bothincludeandexcludesimultaneously. Adding such a case would improve test coverage for the combined scenario.Example entity to add
{ "name": "entityNameWithIncludeExclude", "filters": { "include": ["status", "name"] } + }, + { + "name": "entityNameWithBothFilters", + "filters": { + "include": ["status", "name"], + "exclude": ["search"] + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/schemas/browse.json` around lines 14 - 24, Test coverage is missing a mock entity that has both include and exclude in its filters; add a new JSON entity object (e.g., name "entityNameWithBothIncludeExclude") alongside "entityNameWithIncludeExclude" and "entityNameWithFilters" with a "filters" object containing both "include": ["status","name"] and "exclude": ["internalFlag"] so the combined scenario is exercised, and mirror this new entity in the corresponding browse-countDown mock as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/schemas/common/import-export.js`:
- Around line 12-24: filtersEntitySchema's object branch currently allows an
empty object and empty include/exclude arrays; update the object branch in
filtersEntitySchema to add "minProperties: 1" so an object must contain at least
one property, and add "minItems: 1" to both the include and exclude array
schemas (matching the entityProp pattern) so empty arrays are rejected; change
these constraints on the filtersEntitySchema definition (referencing
filtersEntitySchema, include, and exclude) to enforce meaningful filter content.
In `@tests/mocks/schemas/browse.json`:
- Around line 14-24: Test coverage is missing a mock entity that has both
include and exclude in its filters; add a new JSON entity object (e.g., name
"entityNameWithBothIncludeExclude") alongside "entityNameWithIncludeExclude" and
"entityNameWithFilters" with a "filters" object containing both "include":
["status","name"] and "exclude": ["internalFlag"] so the combined scenario is
exercised, and mirror this new entity in the corresponding browse-countDown mock
as well.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 12
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/mocks/schemas/browse-countDown.json (1)
23-40: Consider adding afilters.include-only entity to this fixture for symmetric coverage.The three new entries cover
filters: false,exclude-only, andinclude + exclude. Aninclude-only case (e.g.,{ "name": "entityIncludeFilters", "filters": { "include": ["status"] } }) is documented as a test case in the PR objectives but is absent from this fixture. Per the AI summary it is present inbrowse.json— adding it here too would makebrowse-countDown.jsonself-contained for the fullfilterssurface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/schemas/browse-countDown.json` around lines 23 - 40, Add an include-only fixture entry to mirror the other filter cases: insert an object with "name": "entityIncludeFilters" and a "filters" object containing only "include": ["status"] so the file has symmetric coverage alongside "entityNoFilters", "entityExcludeFilters", and "entityWithBothIncludeExclude"; ensure the new entry follows the same array/object style as the existing items and is valid JSON (commas between objects).lib/schemas/common/import-export.js (1)
40-45: The string branch ofentitiesSchemais untested forisExport=true.
entitiesSchemais a union ofstringType | exportEntityObjectSchema. The string branch (a bare entity name string as an export entity) is newly introduced by this PR, but allcanExport.entitiesentries in the provided fixtures are objects. The existingcanImportstring fixtures go throughstringTypedirectly (notentitiesSchema), so they don't cover this path. Consider adding at least one plain string entry to acanExport.entitiesarray in a test fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/schemas/common/import-export.js` around lines 40 - 45, entitiesSchema currently unions stringType and exportEntityObjectSchema but the string branch isn't tested for export mode; add at least one plain string entry to a canExport.entities array in the test fixtures so the stringType path is exercised when isExport=true. Update or add a fixture used by the export tests (the file that feeds canExport.entities) to include a bare entity name string, then run the export-related tests to ensure validation passes through entitiesSchema's string branch; reference entitiesSchema, stringType, exportEntityObjectSchema, canExport.entities and isExport when locating code and tests to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/schemas/common/import-export.js`:
- Around line 40-45: entitiesSchema currently unions stringType and
exportEntityObjectSchema but the string branch isn't tested for export mode; add
at least one plain string entry to a canExport.entities array in the test
fixtures so the stringType path is exercised when isExport=true. Update or add a
fixture used by the export tests (the file that feeds canExport.entities) to
include a bare entity name string, then run the export-related tests to ensure
validation passes through entitiesSchema's string branch; reference
entitiesSchema, stringType, exportEntityObjectSchema, canExport.entities and
isExport when locating code and tests to update.
In `@tests/mocks/schemas/browse-countDown.json`:
- Around line 23-40: Add an include-only fixture entry to mirror the other
filter cases: insert an object with "name": "entityIncludeFilters" and a
"filters" object containing only "include": ["status"] so the file has symmetric
coverage alongside "entityNoFilters", "entityExcludeFilters", and
"entityWithBothIncludeExclude"; ensure the new entry follows the same
array/object style as the existing items and is valid JSON (commas between
objects).
1 participant
Link al ticket
https://janiscommerce.atlassian.net/browse/JMV-3997
Descripción del requerimiento
Alinear el schema de validación de import/export con los cambios en Views, en las entidades de
canExportdebe ser posible definir la propiedad opcionalfilterscon valorfalse(no enviar filtros al backend) u objeto conincludey/oexclude(arrays de strings) para indicar qué filtros se envían o excluyenDescripción de la solución
En
lib/schemas/common/import-export.jsse añadió el schemafiltersEntitySchema(oneOf:falseu objeto conincludey/oexclude, arrays de strings) y se integró como propiedad opcional en el objeto de entidad de export. Las entidades pueden seguir siendo un string o un objeto conname,type,format,fieldsy, opcionalmente,filters. En tests se agregaron ejemplos enbrowse.jsonybrowse-countDown.json(y sus expected) con entidades que usanfilters: false,filters.includeyfilters.excludepara validar el schemaCómo se puede probar?
Ejecutar los tests del validador y comprobar los casos de la tabla.
filtersfilters: falsefilters.include(array de strings)filters.exclude(array de strings)filterscon include y excludefiltersinválido (ej. string o array)Changelog
Added - New filters type object
Summary by CodeRabbit
Note
Low Risk
Schema-only change plus test fixture updates; low blast radius aside from potentially rejecting/accepting different
canExportpayload shapes.Overview
Extends the
lib/schemas/common/import-export.jsvalidation forcanExport.entitiesto allow an optionalfiltersproperty per export entity, wherefilterscan befalse(send no filters) or an object withincludeand/orexcludestring arrays.Updates the
browseandbrowse-countDownschema fixtures (and expected outputs) to cover the newfiltersvariants (none,false, include-only, exclude-only, and include+exclude).Written by Cursor Bugbot for commit 7696d54. This will update automatically on new commits. Configure here.