Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughRetargets zod-plugin sources and tests to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (3)
eslint.config.js (2)
242-248: Add vitest setup file to the tests group for consistent lint rules.Including zod-plugin/vitest.setup.ts under the tests/all config keeps rule relaxations consistent with express-zod-api’s setup file.
files: [ "express-zod-api/tests/*.ts", "express-zod-api/vitest.setup.ts", "migration/*.spec.ts", + "zod-plugin/vitest.setup.ts", "zod-plugin/tests/*.ts", ],
177-185: Optional: mirror ignore patterns for zod-plugin.Consider adding coverage output and aligning the trailing slash for consistency.
- Add: "zod-plugin/coverage/"
- Change: "zod-plugin/dist" → "zod-plugin/dist/"
zod-plugin/vitest.setup.ts (1)
1-1: Setup now initializes runtime from src — good.Ensure this file is included by the workspace’s vitest config (it appears to be). Also consider linting it via eslint tests group (see config suggestion).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
eslint.config.js(1 hunks)zod-plugin/src/runtime.ts(1 hunks)zod-plugin/tests/brand.spec.ts(1 hunks)zod-plugin/tests/index.spec.ts(1 hunks)zod-plugin/tests/packer.spec.ts(1 hunks)zod-plugin/tests/runtime.spec.ts(1 hunks)zod-plugin/tsdown.config.ts(1 hunks)zod-plugin/vitest.setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:11:20.768Z
Learning: In the express-zod-api repository, RobinTail follows a release workflow where package.json version is only updated on the master branch after merging all planned release changes. Changelog entries may show future version numbers while package.json remains at the previous version during feature development, and this is intentional workflow, not a version inconsistency that needs to be flagged.
📚 Learning: 2025-08-01T09:48:13.742Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-01T09:48:13.742Z
Learning: In express-zod-api, when migrating from Zod v3 to v4, the correct approach for internal type imports is to change from `import type { $ZodType } from "zod/v4/core"` to `import { z } from "zod"` and then use `z.core.$ZodType`. The zod/v4/core module is reexported as z.core by the main zod package, making this a valid and working approach.
Applied to files:
zod-plugin/tests/packer.spec.tszod-plugin/vitest.setup.tszod-plugin/tests/brand.spec.tszod-plugin/src/runtime.tszod-plugin/tests/index.spec.ts
📚 Learning: 2025-05-27T19:35:57.357Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/tests/buffer-schema.spec.ts:32-37
Timestamp: 2025-05-27T19:35:57.357Z
Learning: In the express-zod-api project, tests are run from the `express-zod-api` workspace directory, and the project uses an ESM-first environment without `__dirname`. Relative paths like `../logo.svg` in test files correctly resolve to the repository root due to this test execution context.
Applied to files:
zod-plugin/tests/packer.spec.tszod-plugin/tests/runtime.spec.tseslint.config.jszod-plugin/tests/brand.spec.tszod-plugin/tests/index.spec.ts
📚 Learning: 2025-08-08T16:45:20.527Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-08T16:45:20.527Z
Learning: express-zod-api/zod-plugin: The ZodType.prototype.brand getter returns setBrand.bind(this) each time (no memoization). setBrand must always call pack() and include { brand: undefined } when unbranding so that pack → .check() runs and prior brand is overwritten; do not suggest removing the explicit undefined entry.
Applied to files:
zod-plugin/tests/packer.spec.tszod-plugin/tests/runtime.spec.tszod-plugin/tests/brand.spec.tszod-plugin/src/runtime.ts
📚 Learning: 2025-08-08T11:59:04.814Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2878
File: zod-plugin/runtime.ts:39-42
Timestamp: 2025-08-08T11:59:04.814Z
Learning: express-zod-api/zod-plugin/runtime.ts (TypeScript): Do not memoize the ZodType.prototype.brand getter. The current design returning setBrand.bind(this) on each access is intentional/preferred; avoid redefining "brand" as a data property per instance.
Applied to files:
zod-plugin/tests/runtime.spec.tszod-plugin/tests/brand.spec.tszod-plugin/src/runtime.ts
📚 Learning: 2025-08-08T16:45:20.527Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-08T16:45:20.527Z
Learning: express-zod-api/zod-plugin: The brand setter must be immutable and always invoke pack→.check() via the getter-bound setBrand. Including brandProperty with an explicit undefined enables unbranding and should not be optimized away.
Applied to files:
zod-plugin/tests/runtime.spec.tszod-plugin/tests/brand.spec.tszod-plugin/src/runtime.ts
📚 Learning: 2025-08-08T16:45:20.527Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-08T16:45:20.527Z
Learning: express-zod-api zod-plugin: The brand setter must stay immutable and always invoke pack→.check() (onattach). Including brandProperty with an explicit undefined enables “unbranding” (overwriting a previous brand) and should not be optimized away.
Applied to files:
zod-plugin/tests/runtime.spec.tszod-plugin/tests/brand.spec.tszod-plugin/src/runtime.ts
📚 Learning: 2025-08-08T16:45:20.527Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-08T16:45:20.527Z
Learning: In express-zod-api zod-plugin, the brand setter must be immutable and always call `.check()` (packer’s onattach). Therefore, writing an explicit undefined entry in the bag for brand is intentional and should not be suggested for removal.
Applied to files:
zod-plugin/tests/runtime.spec.tszod-plugin/tests/brand.spec.ts
📚 Learning: 2025-06-02T21:08:56.475Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:08:56.475Z
Learning: The `cjs-test` directory in the express-zod-api repository is a test workspace and should be excluded when checking for main project version consistency with changelog entries.
Applied to files:
eslint.config.jszod-plugin/tests/index.spec.ts
📚 Learning: 2025-05-27T19:27:13.492Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/tests/form-schema.spec.ts:31-31
Timestamp: 2025-05-27T19:27:13.492Z
Learning: Zod version 3.25.0 and later expose the Zod v4 API through the special import paths "zod/v4" and "zod/v4/core", allowing v4 features like .loose() to be used even when the package.json dependency shows a 3.x version.
Applied to files:
zod-plugin/src/runtime.ts
🔇 Additional comments (6)
zod-plugin/tests/runtime.spec.ts (1)
3-3: Path update to ../src looks good.Importing getBrand from the entrypoint under src aligns with the new layout.
zod-plugin/tests/index.spec.ts (1)
2-2: Entrypoint import redirected to ../src — OK.Test assertions validate exported surface; no further changes needed.
zod-plugin/tsdown.config.ts (1)
5-5: DTS augmentation import verified — build OK, attw clean.tsdown emitted dist/augmentation.d.ts and dist/index.{js,d.ts}; the dts banner import (import './augmentation.js') is intentional for DTS-only augmentation and did not trigger attw.
zod-plugin/tests/packer.spec.ts (1)
2-2: OK to source pack/unpack from ../src.Tests still cover merge semantics and parse behavior; looks consistent.
zod-plugin/tests/brand.spec.ts (1)
2-3: Direct imports from ../src/ are appropriate.*Spy on pack remains valid; unbranding and bag access cases are covered.
zod-plugin/src/runtime.ts (1)
2-2: JSON import path updated — resolveJsonModule enabled; verify bundler inlines JSON for ESM consumers.
- zod-plugin/tsconfig.json: resolveJsonModule = true.
- zod-plugin/package.json: "type": "module", build = "tsdown" — no bundler config found to confirm JSON is inlined; ensure tsdown/your build emits a dist that does not require raw JSON imports at runtime (or emits JSON import assertions).
- zod-plugin/src/runtime.ts imports ../package.json and sets pluginFlag = Symbol.for(name) (zod-plugin/src/runtime.ts:20) — confirm this stable key is intended.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eslint.config.js (1)
178-179: Make ignore globs recurse to ensure contents are excludedUse
**/dist/**and**/coverage/**so all files under those dirs are ignored reliably across tools.- ignores: ["**/dist/", "**/coverage/", "compat-test/sample.ts"], + ignores: ["**/dist/**", "**/coverage/**", "compat-test/sample.ts"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eslint.config.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:11:20.768Z
Learning: In the express-zod-api repository, RobinTail follows a release workflow where package.json version is only updated on the master branch after merging all planned release changes. Changelog entries may show future version numbers while package.json remains at the previous version during feature development, and this is intentional workflow, not a version inconsistency that needs to be flagged.
Learnt from: RobinTail
PR: RobinTail/express-zod-api#0
File: :0-0
Timestamp: 2025-08-01T09:48:13.742Z
Learning: In express-zod-api, when migrating from Zod v3 to v4, the correct approach for internal type imports is to change from `import type { $ZodType } from "zod/v4/core"` to `import { z } from "zod"` and then use `z.core.$ZodType`. The zod/v4/core module is reexported as z.core by the main zod package, making this a valid and working approach.
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/tests/buffer-schema.spec.ts:32-37
Timestamp: 2025-05-27T19:35:57.357Z
Learning: In the express-zod-api project, tests are run from the `express-zod-api` workspace directory, and the project uses an ESM-first environment without `__dirname`. Relative paths like `../logo.svg` in test files correctly resolve to the repository root due to this test execution context.
📚 Learning: 2025-06-02T21:08:56.475Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:08:56.475Z
Learning: The `cjs-test` directory in the express-zod-api repository is a test workspace and should be excluded when checking for main project version consistency with changelog entries.
Applied to files:
eslint.config.js
📚 Learning: 2025-05-27T19:35:57.357Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/tests/buffer-schema.spec.ts:32-37
Timestamp: 2025-05-27T19:35:57.357Z
Learning: In the express-zod-api project, tests are run from the `express-zod-api` workspace directory, and the project uses an ESM-first environment without `__dirname`. Relative paths like `../logo.svg` in test files correctly resolve to the repository root due to this test execution context.
Applied to files:
eslint.config.js
Comment on lines
+237
to
238
| files: ["**/tests/*.ts", "**/vitest.setup.ts", "**/*.spec.ts"], | ||
| rules: { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Broaden test globs to cover nested tests and .test.ts files
Current pattern misses nested files under tests/** and any *.test.ts. Safer to include both.
- files: ["**/tests/*.ts", "**/vitest.setup.ts", "**/*.spec.ts"],
+ files: ["**/tests/**/*.ts", "**/vitest.setup.ts", "**/*.spec.ts", "**/*.test.ts"],Run to check if any .test.ts exist or nested tests would be missed:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Possible misses with current config:"
echo "Count of **/*.test.ts:"
rg --files -g '**/*.test.ts' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/coverage/**' | wc -l
echo "Example nested tests under **/tests/**:"
rg --files -g '**/tests/**/*.ts' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/coverage/**' | head -n 20Length of output: 61
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for .test.ts files:"
rg --files -g '**/*.test.ts' -g '!**/{node_modules,dist,coverage}/**' || true
echo "Checking for nested tests under tests/**:"
rg --files -g '**/tests/**/*.ts' -g '!**/{node_modules,dist,coverage}/**' || trueLength of output: 2128
Broaden test globs in eslint.config.js to include nested tests and .test.ts
Current "/tests/*.ts" misses nested files under tests/ (rg found many); add "/tests//.ts" and "**/.test.ts".
File: eslint.config.js (lines 237-238)
- files: ["**/tests/*.ts", "**/vitest.setup.ts", "**/*.spec.ts"],
+ files: ["**/tests/**/*.ts", "**/vitest.setup.ts", "**/*.spec.ts", "**/*.test.ts"],📝 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.
Suggested change
| files: ["**/tests/*.ts", "**/vitest.setup.ts", "**/*.spec.ts"], | |
| rules: { | |
| files: ["**/tests/**/*.ts", "**/vitest.setup.ts", "**/*.spec.ts", "**/*.test.ts"], | |
| rules: { |
🤖 Prompt for AI Agents
In eslint.config.js around lines 237 to 238, the test file globs are too narrow
("**/tests/*.ts") and miss nested test files and files using the .test.ts
convention; update the files array to include "**/tests/**/*.ts" to capture
nested tests and add a pattern like "**/*.test.ts" so .test.ts files are linted
as well, keeping existing patterns intact and ensuring no duplicates.
Merged
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
1 participant
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
Chores
Tests
Refactor
No user-facing behavior changes.