Feat: Add initial Node FormRequest package#1
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis PR introduces ChangesFormRequest Validation Library
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/formrequest.test.ts (1)
123-135: ⚡ Quick winAvoid order-coupled assertion for
issuesrules.This assertion is brittle against harmless internal ordering changes. Prefer membership + count assertions unless rule order is part of the public contract.
♻️ Suggested test hardening
- expect(result.issues.map((issue) => issue.rule)).toEqual([ + expect(result.issues.map((issue) => issue.rule)).toEqual(expect.arrayContaining([ 'required', 'email', 'number', 'url', 'accepted', 'array', 'object', 'length', 'regex', 'in', 'between' - ]); + ])); + expect(result.issues).toHaveLength(11);🤖 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/formrequest.test.ts` around lines 123 - 135, The test currently asserts exact ordering of result.issues.map(issue => issue.rule), which is brittle; change it to assert membership and count instead: replace the strict order assertion with an assertion that the array contains all expected rule names (use an "arrayContaining"/membership-style assertion against the list ['required','email','number','url','accepted','array','object','length','regex','in','between']) and add a separate assertion that result.issues.length (or the mapped rules length) equals 11 so the test verifies presence and count but not order; locate the assertion in tests/formrequest.test.ts that references result.issues.map((issue) => issue.rule) and update it accordingly.src/rules/index.ts (1)
89-99: ⚡ Quick winConsider the POSITIVE_INFINITY fallback behavior for non-measurable types.
The
measurableLengthhelper returnsNumber.POSITIVE_INFINITYfor types that don't have a measurable length (objects, dates, etc.). This means:
min(10)will pass for objects/dates (∞ >= 10)max(10)will fail for objects/dates (∞ > 10)between(5, 10)will fail for objects/dates (∞ > 10)length(10)will fail for objects/dates (∞ ≠ 10)While this behavior might be intentional, the asymmetry could be counterintuitive—
minconstraints pass for non-measurable values whilemax/between/lengthfail. Consider whether returning0or explicitly failing validation would be clearer.If this design is intentional, document it so users understand that length constraints are meant for numbers, strings, and arrays only.
🤖 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/rules/index.ts` around lines 89 - 99, The measurableLength helper currently returns Number.POSITIVE_INFINITY for non-measurable types causing asymmetric validation; change measurableLength to return NaN for any type that isn't number, string, or array, then update the validators that call measurableLength (e.g., the min, max, between, length validation functions) to treat NaN as an explicit non-measurable value and fail validation (or return a clear "non-measurable" error) instead of comparing against Infinity; also add a short comment/docstring near measurableLength explaining that non-measurable types produce NaN and are considered validation failures for length-based rules.package.json (1)
3-3: ⚡ Quick winUpdate version before publishing to npm.
Version
0.0.0is a pre-release placeholder. Consider updating to0.1.0or1.0.0before the first npm publish.Would you like me to open an issue to track the version update task?
🤖 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 `@package.json` at line 3, Update the "version" field in package.json from the placeholder "0.0.0" to an appropriate release version (e.g., "0.1.0" or "1.0.0") before publishing to npm; locate the "version" key in package.json and bump it accordingly, and optionally create or link an issue to track future versioning and release notes.tsconfig.json (1)
9-18: 💤 Low valueConsider aligning with standard library TypeScript project structure.
The current setup uses
rootDir: "."and includes config files (vitest.config.ts,eslint.config.js) in the TypeScript project. While this works becausetsuphandles compilation independently, it's non-standard for libraries:
- Standard pattern:
rootDir: "src",include: ["src"]keeps the TypeScript project focused on application code- Current pattern: Including the entire project makes it unclear what's part of the distributable library
Since
tscis only used for type-checking (--noEmit), this doesn't affect the build output, but it may confuse contributors expecting typical library structure.♻️ Suggested standard structure
{ "compilerOptions": { "declaration": true, "exactOptionalPropertyTypes": true, "module": "NodeNext", "moduleResolution": "NodeNext", "noUncheckedIndexedAccess": true, "outDir": "dist", - "rootDir": ".", + "rootDir": "src", "strict": true, "target": "ES2022" }, "include": [ - "src", - "tests", - "vitest.config.ts", - "eslint.config.js" + "src" ] }Note: Tests and config files would still be type-checked if they import from
src, but wouldn't be part of the primary project compilation.🤖 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 `@tsconfig.json` around lines 9 - 18, Update tsconfig.json to use a standard library layout by setting "rootDir" to "src" and restricting the "include" array to only "src" (remove entries like "tests", "vitest.config.ts", "eslint.config.js"); this keeps type checking focused on source files and avoids treating test/config files as part of the primary project compilation while still allowing them to import from src for type-checking.
🤖 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 `@package.json`:
- Around line 47-54: The peer dependency for Express is too broad and may allow
incompatible Express 5.x; update package.json's "peerDependencies" entry for
"express" to ">=4.18 <5" to exclude v5, or alternatively add explicit
compatibility tests that exercise your ExpressLikeRequest types against both
Express 4.x and 5.x (covering req.body, req.query, req.params, and removal of
req.param) and only keep the current range if those tests pass; ensure
references to the ExpressLikeRequest type reflect any behavior differences
discovered.
---
Nitpick comments:
In `@package.json`:
- Line 3: Update the "version" field in package.json from the placeholder
"0.0.0" to an appropriate release version (e.g., "0.1.0" or "1.0.0") before
publishing to npm; locate the "version" key in package.json and bump it
accordingly, and optionally create or link an issue to track future versioning
and release notes.
In `@src/rules/index.ts`:
- Around line 89-99: The measurableLength helper currently returns
Number.POSITIVE_INFINITY for non-measurable types causing asymmetric validation;
change measurableLength to return NaN for any type that isn't number, string, or
array, then update the validators that call measurableLength (e.g., the min,
max, between, length validation functions) to treat NaN as an explicit
non-measurable value and fail validation (or return a clear "non-measurable"
error) instead of comparing against Infinity; also add a short comment/docstring
near measurableLength explaining that non-measurable types produce NaN and are
considered validation failures for length-based rules.
In `@tests/formrequest.test.ts`:
- Around line 123-135: The test currently asserts exact ordering of
result.issues.map(issue => issue.rule), which is brittle; change it to assert
membership and count instead: replace the strict order assertion with an
assertion that the array contains all expected rule names (use an
"arrayContaining"/membership-style assertion against the list
['required','email','number','url','accepted','array','object','length','regex','in','between'])
and add a separate assertion that result.issues.length (or the mapped rules
length) equals 11 so the test verifies presence and count but not order; locate
the assertion in tests/formrequest.test.ts that references
result.issues.map((issue) => issue.rule) and update it accordingly.
In `@tsconfig.json`:
- Around line 9-18: Update tsconfig.json to use a standard library layout by
setting "rootDir" to "src" and restricting the "include" array to only "src"
(remove entries like "tests", "vitest.config.ts", "eslint.config.js"); this
keeps type checking focused on source files and avoids treating test/config
files as part of the primary project compilation while still allowing them to
import from src for type-checking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2812118-363a-4a9d-9f3d-04ece6a49e49
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.aiignore.github/pull_request_template.md.github/skills/formrequest-package/SKILL.md.github/workflows/ci.yml.gitignore.humanignoreAGENTS.mdCHANGELOG.mdCLAUDE.mdLICENSEREADME.mdai/skills/README.mdai/skills/USAGE.mddocs/README.mddocs/ai-skills.mddocs/ci-cd.mddocs/release-process.mdeslint.config.jspackage.jsonsrc/FormRequest.tssrc/index.tssrc/rules/createRule.tssrc/rules/index.tssrc/types.tstests/formrequest.test.tstsconfig.jsonvitest.config.ts
1 participant
Summary
Validation
Coverage: 100% statements, branches, functions, and lines.
Summary by CodeRabbit
New Features
Documentation
Chores