Conversation
📝 WalkthroughWalkthroughError handling and validation are enhanced through new utility methods for normalizing error output. A helper function formats error items in index.js, a static method standardizes AJV error formatting in Validator, and tests are refactored from hardcoded assertions to a data-driven framework with reusable schema loading and diff comparison utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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 |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 12. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
![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
🧹 Nitpick comments (4)
index.js (1)
101-105: Prefer.forEach()over.map()for side-effect-only iteration.Line 104 uses
.map()to calllogger.error, but the returned array is discarded..forEach()communicates intent more clearly and avoids an unnecessary allocation.♻️ Suggested diff
if(error.errors) { error.errors .map(mapErrorToLogLine) - .map(errorLine => logger.error(errorLine)); + .forEach(errorLine => logger.error(errorLine)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 101 - 105, The error handling code uses .map() twice for side effects and discards the results; replace the second .map() that calls logger.error with .forEach() (and you can also replace the first .map()->.forEach() if you prefer a single-step approach) so that error.errors is transformed via mapErrorToLogLine and each resulting line is logged via logger.error without allocating an unused array; update the block around error.errors, mapErrorToLogLine and logger.error accordingly to reflect intent.tests/validator-test.js (2)
158-164: Inconsistent language: Spanish text in English codebase.Lines 161 and 176 contain Spanish truncation notes (
"diffs adicionales truncados","errores de validacion truncados"), while the rest of the codebase and comments are in English.♻️ Suggested diff
const truncationNote = diffs.length >= MAX_DIFFS_PER_SCHEMA - ? '\n... (diffs adicionales truncados)' + ? '\n... (additional diffs truncated)' : '';And similarly in
formatSchemaExecutionErrorReport:const validationTruncationNote = Array.isArray(error && error.errors) && error.errors.length > MAX_DIFFS_PER_SCHEMA - ? '\n... (errores de validacion truncados)' + ? '\n... (validation errors truncated)' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/validator-test.js` around lines 158 - 164, The truncation note strings in formatSchemaDiffReport and formatSchemaExecutionErrorReport are in Spanish; change them to English for consistency (replace '\n... (diffs adicionales truncados)' with something like '\n... (additional diffs truncated)' and replace the validation error truncation message in formatSchemaExecutionErrorReport ('\n... (errores de validacion truncados)') with an English equivalent such as '\n... (validation errors truncated)') so all messages in these functions are in English; update only the literal strings in formatSchemaDiffReport and formatSchemaExecutionErrorReport.
312-319: All schema cases in a single test — tradeoff worth noting.Running all 25 schemas in one test via
runCompilationAndCollectDiffsgives a consolidated report, but a systemic failure produces a single massive error rather than 25 individual test failures. This makes it harder to track regressions in CI (no per-schema pass/fail granularity). The current approach is acceptable for catching fixture mismatches in aggregate, but consider whether parameterized individual tests (e.g., with a loop generatingit(...)blocks) would give better CI signal long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/validator-test.js` around lines 312 - 319, Current test bundles all 25 schemas into one it() using runCompilationAndCollectDiffs(SCHEMA_COMPILATION_SCHEMAS), which yields a single large failure on systemic breakage; change the test to generate a separate it(...) per schema (iterate SCHEMA_COMPILATION_SCHEMAS with forEach or a for loop inside the describe('schema compilation vs expected') block), call runCompilationAndCollectDiffs or an adapted helper per single schema entry, and assert/fail per test so each schema (identified by its fixture name) reports pass/fail independently; keep the existing failure formatting but include the schema identifier in each test's assertion message to aid CI diagnostics.lib/validator.js (1)
13-21:formatAjvErrors(null)would bypass the default and throw.The default parameter
errors = []only applies whenerrorsisundefined, notnull. Since AJV v6 setsajv.errorstonullon success, a future caller passingnulldirectly would causenull.map()to throw. Currently safe because line 65 guards against it, but consider a defensive normalization.🛡️ Defensive normalization
static formatAjvErrors(errors = []) { - return errors.map((currentError, index) => { + return (errors || []).map((currentError, index) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validator.js` around lines 13 - 21, The formatAjvErrors function can receive null (AJV uses null for no errors) which bypasses the default parameter and causes a crash when calling .map; in formatAjvErrors normalize the incoming errors (inside the static method) to an array first—e.g., use Array.isArray(errors) ? errors : [] or coerce with errors || []—then iterate over that normalized array so formatAjvErrors and callers are safe when passed null or undefined; update the implementation of formatAjvErrors accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/validator-test.js`:
- Around line 171-184: The template builds validationLines as an array which
gets coerced to a comma-separated string when interpolated; inside
formatSchemaExecutionErrorReport, change the handling of validationLines so it
becomes a single string (e.g., join the mapped lines with '' or '\n') instead of
leaving it as an array — update the code that defines validationLines (and any
use in the final template) in formatSchemaExecutionErrorReport to call
.join(...) after .map(...) so the validation error lines render without unwanted
commas while preserving MAX_DIFFS_PER_SCHEMA truncation logic and the
validationTruncationNote behavior.
---
Nitpick comments:
In `@index.js`:
- Around line 101-105: The error handling code uses .map() twice for side
effects and discards the results; replace the second .map() that calls
logger.error with .forEach() (and you can also replace the first
.map()->.forEach() if you prefer a single-step approach) so that error.errors is
transformed via mapErrorToLogLine and each resulting line is logged via
logger.error without allocating an unused array; update the block around
error.errors, mapErrorToLogLine and logger.error accordingly to reflect intent.
In `@lib/validator.js`:
- Around line 13-21: The formatAjvErrors function can receive null (AJV uses
null for no errors) which bypasses the default parameter and causes a crash when
calling .map; in formatAjvErrors normalize the incoming errors (inside the
static method) to an array first—e.g., use Array.isArray(errors) ? errors : []
or coerce with errors || []—then iterate over that normalized array so
formatAjvErrors and callers are safe when passed null or undefined; update the
implementation of formatAjvErrors accordingly.
In `@tests/validator-test.js`:
- Around line 158-164: The truncation note strings in formatSchemaDiffReport and
formatSchemaExecutionErrorReport are in Spanish; change them to English for
consistency (replace '\n... (diffs adicionales truncados)' with something like
'\n... (additional diffs truncated)' and replace the validation error truncation
message in formatSchemaExecutionErrorReport ('\n... (errores de validacion
truncados)') with an English equivalent such as '\n... (validation errors
truncated)') so all messages in these functions are in English; update only the
literal strings in formatSchemaDiffReport and formatSchemaExecutionErrorReport.
- Around line 312-319: Current test bundles all 25 schemas into one it() using
runCompilationAndCollectDiffs(SCHEMA_COMPILATION_SCHEMAS), which yields a single
large failure on systemic breakage; change the test to generate a separate
it(...) per schema (iterate SCHEMA_COMPILATION_SCHEMAS with forEach or a for
loop inside the describe('schema compilation vs expected') block), call
runCompilationAndCollectDiffs or an adapted helper per single schema entry, and
assert/fail per test so each schema (identified by its fixture name) reports
pass/fail independently; keep the existing failure formatting but include the
schema identifier in each test's assertion message to aid CI diagnostics.
| const formatSchemaExecutionErrorReport = (schemaCase, error) => { | ||
| const validationLines = Array.isArray(error && error.errors) | ||
| ? error.errors.slice(0, MAX_DIFFS_PER_SCHEMA).map(line => `${line}\n`) | ||
| : ''; | ||
| const validationTruncationNote = Array.isArray(error && error.errors) && error.errors.length > MAX_DIFFS_PER_SCHEMA | ||
| ? '\n... (errores de validacion truncados)' | ||
| : ''; | ||
|
|
||
| return ( | ||
| `[SCHEMA CASE] ${schemaCase.schema.path} -> ${schemaCase.expected}\n` + | ||
| `Error message: ${(error && error.message) || 'Unknown execution error'}` + | ||
| (validationLines ? `\nvalidation errors:\n${validationLines}${validationTruncationNote}` : '') | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Bug: Array-to-string coercion inserts unwanted commas between validation error lines.
On line 173, error.errors.slice(...).map(line => ...) produces an array. When this array is interpolated into the template literal on line 182, JavaScript calls .toString() (i.e., .join(',')) — inserting commas between each error line.
🐛 Proposed fix
const validationLines = Array.isArray(error && error.errors)
- ? error.errors.slice(0, MAX_DIFFS_PER_SCHEMA).map(line => `${line}\n`)
+ ? error.errors.slice(0, MAX_DIFFS_PER_SCHEMA).map(line => `${line}\n`).join('')
: '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const formatSchemaExecutionErrorReport = (schemaCase, error) => { | |
| const validationLines = Array.isArray(error && error.errors) | |
| ? error.errors.slice(0, MAX_DIFFS_PER_SCHEMA).map(line => `${line}\n`) | |
| : ''; | |
| const validationTruncationNote = Array.isArray(error && error.errors) && error.errors.length > MAX_DIFFS_PER_SCHEMA | |
| ? '\n... (errores de validacion truncados)' | |
| : ''; | |
| return ( | |
| `[SCHEMA CASE] ${schemaCase.schema.path} -> ${schemaCase.expected}\n` + | |
| `Error message: ${(error && error.message) || 'Unknown execution error'}` + | |
| (validationLines ? `\nvalidation errors:\n${validationLines}${validationTruncationNote}` : '') | |
| ); | |
| }; | |
| const formatSchemaExecutionErrorReport = (schemaCase, error) => { | |
| const validationLines = Array.isArray(error && error.errors) | |
| ? error.errors.slice(0, MAX_DIFFS_PER_SCHEMA).map(line => `${line}\n`).join('') | |
| : ''; | |
| const validationTruncationNote = Array.isArray(error && error.errors) && error.errors.length > MAX_DIFFS_PER_SCHEMA | |
| ? '\n... (errores de validacion truncados)' | |
| : ''; | |
| return ( | |
| `[SCHEMA CASE] ${schemaCase.schema.path} -> ${schemaCase.expected}\n` + | |
| `Error message: ${(error && error.message) || 'Unknown execution error'}` + | |
| (validationLines ? `\nvalidation errors:\n${validationLines}${validationTruncationNote}` : '') | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/validator-test.js` around lines 171 - 184, The template builds
validationLines as an array which gets coerced to a comma-separated string when
interpolated; inside formatSchemaExecutionErrorReport, change the handling of
validationLines so it becomes a single string (e.g., join the mapped lines with
'' or '\n') instead of leaving it as an array — update the code that defines
validationLines (and any use in the final template) in
formatSchemaExecutionErrorReport to call .join(...) after .map(...) so the
validation error lines render without unwanted commas while preserving
MAX_DIFFS_PER_SCHEMA truncation logic and the validationTruncationNote behavior.
1 participant
Link al ticket
Descripción del requerimiento
Descripción de la solución
Cambios en
lib/validator.jsValidar respuesta que recibe el back cuando el schema no ocincide con el esperado
Cambios en
index.jsRefactor de
tests/validator-test.jsFunciones
Comportamiento anterior vs actual
Mapeo de cobertura de tests (antes → ahora)
No se perdió cobertura; los escenarios se mantienen con nombres y estructura más claros
Cómo se puede probar?
npm run testerror.errorsarray de strings legiblesDatos extra a tener en cuenta
Changelog
Summary by CodeRabbit
Release Notes