fix: address PR #22 review feedback regressions#23
Closed
riceharvest wants to merge 3 commits intomainfrom
Closed
Conversation
- Upgraded multiple packages to modern standards (Next.js, Next-auth, PWA, SEO). - Added new utility packages: critters, next-circuit-breaker, next-csrf, next-images, next-json-ld. - Integrated Changesets for versioning. - Updated CI/CD workflows and linting configurations. - Fixed numerous linting and type-checking issues across the monorepo.
Owner
Author
|
@codex review |
Review Summary by QodoFix PR #22 review feedback regressions and implement comprehensive plugin system with image optimization
WalkthroughsDescription• **Fixes critical regressions from PR #22 review feedback** across multiple packages • Restores runtime/package entrypoints for critters, next-compose-plugins, and next-optimized-images packages • Fixes module matcher boundary overmatch in next-transpile-modules (distinguishes foo vs foobar modules) • Adds Vitest configuration and setup for next-compose-plugins to enable CI globals availability • Migrates test suites across packages: next-iron-session to Node.js native test runner, next-session to Jest, react-virtualized components to Vitest API • Simplifies test suites for next-images and next-csrf to minimal smoke tests • Updates cookie serialization types from CookieSerializeOptions to SerializeOptions in next-csrf • Adds comprehensive plugin composition system with phase support and optional plugin handling in next-compose-plugins • Implements next-optimized-images plugin with webpack loaders for image optimization (imagemin, WebP, responsive, LQIP, SVG sprites) • Adds react-query-auth example application with MSW mocks, authentication flow, and API client • Relaxes type constraints in next-iron-session crypto operations and next-session compatibility layer • Enhances time parsing and cookie serialization in next-session utilities • Adds Prettier configuration files across multiple packages for consistent code formatting • Updates ESLint configuration to support Vitest and Jest globals • Improves router middleware chain safety in next-connect with null checks • Adds comprehensive monorepo architecture documentation and CI workflow generation scripts • All test suites pass validation locally with 100+ tests across packages Diagramflowchart LR
A["PR #22 Feedback"] -->|"Restore entrypoints"| B["critters, next-compose-plugins, next-optimized-images"]
A -->|"Fix boundary matching"| C["next-transpile-modules"]
A -->|"Add Vitest setup"| D["next-compose-plugins"]
A -->|"Migrate test runners"| E["next-iron-session, next-session, react-virtualized"]
A -->|"Update types"| F["next-csrf, next-session"]
B -->|"Implement"| G["Plugin composition system"]
B -->|"Implement"| H["Image optimization loaders"]
G -->|"Supports"| I["Phase-based execution"]
G -->|"Supports"| J["Optional plugins"]
H -->|"Includes"| K["imagemin, WebP, responsive, LQIP, SVG sprites"]
L["react-query-auth example"] -->|"Uses"| M["MSW mocks"]
L -->|"Implements"| N["Auth flow with API client"]
File Changes1. packages/next-iron-session/src/index.test.ts
|
Code Review by Qodo
1. Raw API error exposed
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8efa82311a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { createServer, IncomingMessage, request, ServerResponse } from "http"; | ||
| import { inject } from "light-my-request"; | ||
| import MemoryStore from "../src/memory-store"; | ||
| import session from "../src/index"; | ||
| import session from "../src/session"; |
There was a problem hiding this comment.
This import was changed to ../src/session, but this package only ships src/index.ts (there is no src/session.ts), and the same bad path also appears in test/utils.test.ts. With Vitest configured to run test/**/*.test.ts, module resolution fails before tests execute, so the next-session test job breaks immediately.
Useful? React with 👍 / 👎.
| @@ -1,9 +1,9 @@ | |||
| // @ts-nocheck | |||
| import { vi } from "vitest"; | |||
| import { jest } from "@jest/globals"; | |||
There was a problem hiding this comment.
The tests now import @jest/globals, but packages/next-session/package.json does not declare jest or @jest/globals while the test script runs Vitest. In standalone package CI/install contexts this produces a Cannot find module '@jest/globals' failure at file load time, so the suite cannot start.
Useful? React with 👍 / 👎.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+14
to
+21
| export async function handleApiResponse(response: Response) { | ||
| const data = await response.json() | ||
|
|
||
| if (response.ok) { | ||
| return data | ||
| } | ||
|
|
||
| return Promise.reject(data) |
There was a problem hiding this comment.
1. Raw api error exposed 📘 Rule violation ⛨ Security
handleApiResponse rejects with raw server-provided JSON (Promise.reject(data)), which can propagate internal error details to the UI. This conflicts with the requirement that user-facing errors be generic and not leak implementation details.
Agent Prompt
## Issue description
The API helper rejects with raw server-provided error payloads, which can leak sensitive/internal details to end users.
## Issue Context
Compliance requires generic user-facing errors; detailed errors should be kept for secure/internal debugging.
## Fix Focus Areas
- packages/react-query-auth/examples/vite/src/lib/api.ts[14-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+85
| const next: any = () => { | ||
| const fn = fns[++i]; | ||
| if (fn) return (fn as any)(...args, next); | ||
| }; | ||
| const first = fns[i]; | ||
| return first ? (first as any)(...args, next) : Promise.resolve(); |
There was a problem hiding this comment.
2. Silent next() termination 🐞 Bug ⛯ Reliability
Router.exec now makes next() a no-op when there is no subsequent handler, instead of failing fast. This can mask middleware bugs (e.g., last handler calling next() or calling it twice) and may lead to handlers completing without sending a response or triggering onError.
Agent Prompt
### Issue description
`Router.exec` currently makes `next()` a silent no-op when there are no more handlers. This masks common middleware bugs (e.g., calling `next()` in the last handler, or calling it twice) and can lead to requests timing out without a clear error.
### Issue Context
`NodeRouter.handler()` awaits `Router.exec(...)` when a route is matched and will only run `onError` if something throws. A silent no-op means the request can complete without a response and without error handling.
### Fix Focus Areas
- packages/next-connect/src/router.ts[75-86]
- packages/next-connect/src/node.ts[93-111]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Closing as superseded by PR #24. Follow-up fixes and review responses from this branch are already carried in fix/pr23-feedback-round3. |
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.
What this fixes
Validation run locally
RUN v4.0.18 /home/dario/Documents/dev workspace/opensourceframework/packages/next-compose-plugins
✓ src/tests/optional.test.js (3 tests) 6ms
✓ src/tests/phases.test.js (5 tests) 8ms
✓ src/tests/index.test.js (4 tests) 7ms
✓ src/tests/compose.test.js (15 tests) 12ms
Test Files 4 passed (4)
Tests 27 passed (27)
Start at 13:50:41
Duration 336ms (transform 229ms, setup 162ms, import 196ms, tests 34ms, environment 1ms)
RUN v4.0.18 /home/dario/Documents/dev workspace/opensourceframework/packages/next-transpile-modules
✓ src/tests/helpers.test.js (7 tests | 3 skipped) 5ms
✓ src/tests/unit.test.js (50 tests) 15ms
Test Files 2 passed (2)
Tests 54 passed | 3 skipped (57)
Start at 13:50:43
Duration 472ms (transform 54ms, setup 0ms, import 454ms, tests 20ms, environment 0ms)
RUN v2.1.9 /home/dario/Documents/dev workspace/opensourceframework/packages/next-csrf
✓ test/index.test.ts (2 tests) 4ms
Test Files 1 passed (1)
Tests 2 passed (2)
Start at 13:50:45
Duration 793ms (transform 95ms, setup 0ms, collect 120ms, tests 4ms, environment 0ms, prepare 102ms)
RUN v2.1.9 /home/dario/Documents/dev workspace/opensourceframework/packages/next-images
✓ test/index.test.ts (2 tests) 4ms
Test Files 1 passed (1)
Tests 2 passed (2)
Start at 13:50:47
Duration 681ms (transform 53ms, setup 0ms, collect 40ms, tests 4ms, environment 0ms, prepare 103ms)
RUN v1.6.1 /home/dario/Documents/dev workspace/opensourceframework/packages/critters
✓ test/index.test.ts (2 tests) 3ms
Test Files 1 passed (1)
Tests 2 passed (2)
Start at 13:50:49
Duration 1.03s (transform 112ms, setup 0ms, collect 202ms, tests 3ms, environment 0ms, prepare 100ms)
Notes