feat(critters): improve animation parsing and font preloading#29
Closed
riceharvest wants to merge 60 commits intomainfrom
Closed
feat(critters): improve animation parsing and font preloading#29riceharvest wants to merge 60 commits intomainfrom
riceharvest wants to merge 60 commits intomainfrom
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.
- Remove legacy NextAuth adapters and resolve workspace version conflicts - Clean up test warning noise and fix tsconfig/jest setups for next-auth - Update Workbox/Terser dependencies in next-pwa to align with workspace - Synchronize root lockfile to reflect nested package resolutions
Fixes `JWT_AUTO_GENERATED_SIGNING_KEY` and `JWT_AUTO_GENERATED_ENCRYPTION_KEY` warnings properly by supplying JWKs directly in the test suite rather than mocking the logger.
Review Summary by QodoComprehensive monorepo modernization with critters improvements, test framework migrations, and plugin system implementation
WalkthroughsDescription• **Font preloading improvements**: Fixed font preloading to parse multiple font sources from a single src property with format detection, and added type attribute to font preload links when format is specified • **Animation parsing enhancements**: Improved animation name extraction by filtering out CSS keywords and numeric values • **Container detection robustness**: Added fallback logic for malformed data-critters-container attributes in critters DOM handling • **Comprehensive test suite expansion**: Added extensive test coverage across multiple packages including MDX node retrieval, CSRF middleware, plugin composition, JWT signing/encryption, and image optimization • **Test framework migrations**: Migrated multiple test suites from Jest to vitest and from vitest to Node.js built-in test module • **Plugin composition system**: Implemented complete plugin composition framework with phase support, optional plugin resolution, and configuration merging • **Image optimization plugin**: Added full image optimization plugin implementation with loader detection, webpack integration, and resource query configuration • **Next.js 13+ support**: Added version detection for Next.js to use native transpilePackages for v13+ with improved error handling • **Type safety improvements**: Enhanced type safety in session implementation with explicit type imports and proper type casting • **Legacy Prisma adapter**: Implemented new legacy Prisma authentication adapter with configurable model mapping • **MDX utilities**: Added comprehensive MDX node handling utilities with caching mechanism via SHA256 hashes • **Test suite simplifications**: Consolidated and simplified test suites in several packages to focus on core functionality • **Documentation cleanup**: Removed deprecation warnings and simplified documentation in next-images package Diagramflowchart LR
A["Critters Runtime<br/>Font & Animation"] -->|"Improved parsing"| B["Enhanced Preloading"]
C["Test Frameworks<br/>Jest/Vitest/Node"] -->|"Migration"| D["Unified Test Suite"]
E["Plugin System<br/>Composition"] -->|"Implementation"| F["Phase-aware Execution"]
G["Image Optimization<br/>Loaders"] -->|"Detection & Config"| H["Webpack Integration"]
I["Next.js Support<br/>v13+"] -->|"Version Detection"| J["Native Transpilation"]
K["Type Safety<br/>Improvements"] -->|"Enhanced Casting"| L["Session Management"]
File Changes1. packages/next-images/test/index.test.ts
|
Code Review by Qodo
1. next-optimized-images code in lib/
|
![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: 9e8f94dacf
ℹ️ 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".
Comment on lines
750
to
+752
| if (decl.prop === 'animation' || decl.prop === 'animation-name') { | ||
| // Filter out common animation keywords and durations | ||
| const keywords = ['none', 'inherit', 'initial', 'unset', 'infinite', 'alternate', 'alternate-reverse', 'forwards', 'backwards', 'both', 'running', 'paused', 'normal', 'reverse', 'ease', 'ease-in', 'ease-out', 'ease-in-out', 'linear', 'step-start', 'step-end']; |
There was a problem hiding this comment.
The new keyword filter is applied to both animation and animation-name, but animation-name accepts custom identifiers that can legally be words like linear, reverse, or forwards. With this change those names are dropped from criticalKeyframeNames, so the matching @keyframes block is removed in the second pass and the animation stops working in the inlined CSS. Restrict shorthand keyword filtering to animation parsing, or use a narrower rule for animation-name.
Useful? React with 👍 / 👎.
Comment on lines
+820
to
+822
| if (format) { | ||
| preload.setAttribute('type', `font/${format}`); | ||
| } |
There was a problem hiding this comment.
The preload type is now derived as font/${format}, but CSS format() tokens are not MIME subtypes (for example truetype, opentype, and embedded-opentype do not map directly to valid font/* values). This can produce invalid type attributes, and browsers may skip those preloads, regressing font preload behavior for non-woff sources. Add an explicit format-to-MIME map and omit type when the format is unknown.
Useful? React with 👍 / 👎.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+3
| const path = require('path'); | ||
| const fs = require('fs'); | ||
|
|
There was a problem hiding this comment.
1. next-optimized-images code in lib/ 📘 Rule violation ⛯ Reliability
New package implementation code was added under packages/next-optimized-images/lib/... instead of packages/<name>/src, which breaks the required monorepo layout expectations. This can cause tooling/CI and contributor workflows to miss or inconsistently treat package source.
Agent Prompt
## Issue description
New `next-optimized-images` implementation code is being added under `packages/next-optimized-images/lib/...`, but the monorepo rule requires package implementation code to live under `packages/<name>/src`.
## Issue Context
Keeping source code under `src/` is required so tooling and contributors can reliably find, lint, typecheck, and build package source.
## Fix Focus Areas
- packages/next-optimized-images/lib/loaders/file-loader.js[1-89]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,116 @@ | |||
| const withOptimizedImages = require('../lib'); | |||
There was a problem hiding this comment.
2. tests added at package root 📘 Rule violation ⛯ Reliability
New tests were added under packages/next-optimized-images/__tests__/..., which is not one of the allowed test locations (test/, src/**/*.test.*, or src/__tests__/). This can lead to inconsistent test discovery and deviates from the required monorepo layout.
Agent Prompt
## Issue description
New test files were introduced under `packages/next-optimized-images/__tests__/...`, but the repo standard only allows tests under `test/`, `src/**/*.test.*`, or `src/__tests__/`.
## Issue Context
Test discovery and tooling consistency rely on tests being placed in the documented locations.
## Fix Focus Areas
- packages/next-optimized-images/__tests__/index.test.js[1-116]
- packages/next-optimized-images/__tests__/loaders/index.test.js[1-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,116 @@ | |||
| const withOptimizedImages = require('../lib'); | |||
There was a problem hiding this comment.
3. New tests use .test.js 📘 Rule violation ⛯ Reliability
This PR adds new test files named *.test.js, but the repository requires new tests to be named *.test.ts or *.test.tsx. This can break consistent test conventions and discovery expectations.
Agent Prompt
## Issue description
New tests are being added as `*.test.js`, but the repository requires new test files to be named `*.test.ts` or `*.test.tsx`.
## Issue Context
Consistent test naming is required for predictable discovery and enforcement across packages.
## Fix Focus Areas
- packages/next-optimized-images/__tests__/index.test.js[1-116]
- packages/next-optimized-images/__tests__/loaders/index.test.js[1-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+10
| import { defineConfig } from "vitest/config" | ||
|
|
||
| export default defineConfig({ | ||
| test: { | ||
| globals: true, | ||
| env: { | ||
| NEXTAUTH_URL: "http://localhost:3000/api/auth", | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
4. vitest.config.js not prettier 📘 Rule violation ✓ Correctness
The new vitest.config.js uses double quotes and omits semicolons, diverging from the repository Prettier standards. This introduces inconsistent formatting in newly added code.
Agent Prompt
## Issue description
The newly added `vitest.config.js` does not match the repo’s Prettier conventions (it uses double quotes and omits semicolons).
## Issue Context
Repository formatting standards require single quotes and semicolons for consistency.
## Fix Focus Areas
- vitest.config.js[1-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // detect used keyframes | ||
| if (decl.prop === 'animation' || decl.prop === 'animation-name') { | ||
| // Filter out common animation keywords and durations | ||
| const keywords = ['none', 'inherit', 'initial', 'unset', 'infinite', 'alternate', 'alternate-reverse', 'forwards', 'backwards', 'both', 'running', 'paused', 'normal', 'reverse', 'ease', 'ease-in', 'ease-out', 'ease-in-out', 'linear', 'step-start', 'step-end']; |
There was a problem hiding this comment.
5. keywords line exceeds 100 📘 Rule violation ✓ Correctness
The newly added keywords array is on a single extremely long line, exceeding the 100-character line length requirement. This reduces readability and violates the documented formatting standards.
Agent Prompt
## Issue description
A newly added `keywords` array in `runtime.js` exceeds the repo’s 100-character line length standard.
## Issue Context
Long lines reduce readability and violate the documented Prettier formatting expectations.
## Fix Focus Areas
- packages/critters/src/runtime.js[751-756]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+809
to
+826
| if (src && shouldPreloadFonts) { | ||
| const urlRegex = /url\s*\(\s*(['"]?)(.+?)\1\s*\)(\s*format\s*\(\s*(['"]?)(.+?)\4\s*\))?/g; | ||
| let match; | ||
| while ((match = urlRegex.exec(src)) !== null) { | ||
| const fontUrl = match[2].trim(); | ||
| const format = match[5]; | ||
| if (!preloadedFonts.has(fontUrl)) { | ||
| preloadedFonts.add(fontUrl); | ||
| const preload = document.createElement('link'); | ||
| preload.setAttribute('rel', 'preload'); | ||
| preload.setAttribute('as', 'font'); | ||
| if (format) { | ||
| preload.setAttribute('type', `font/${format}`); | ||
| } | ||
| preload.setAttribute('crossorigin', 'anonymous'); | ||
| preload.setAttribute('href', fontUrl); | ||
| document.head.appendChild(preload); | ||
| } |
There was a problem hiding this comment.
6. Font preload head crash 🐞 Bug ⛯ Reliability
In packages/critters/src/runtime.js, the new @font-face preloading loop unconditionally calls document.head.appendChild(preload), which throws if the parsed HTML has no <head>. This causes Critters.process() to fail for inputs without a head element when fonts/preloadFonts is enabled and a @font-face rule is encountered.
Agent Prompt
### Issue description
`processStyle()` can crash during font preloading because it unconditionally uses `document.head.appendChild(preload)` even though `document.head` can be null in this DOM implementation when HTML lacks a `<head>`.
### Issue Context
Critters’ `document.head` is implemented as `querySelector('head')`, so it is not guaranteed to exist for malformed/minimal HTML. The new font preload parsing loop should be as defensive as other style insertion paths.
### Fix Focus Areas
- packages/critters/src/runtime.js[809-826]
- packages/critters/src/dom.js[216-220]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
|
3 similar comments
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Prevent crash when parsed HTML has no <head> element during font preloading. Uses same fallback pattern as stylesheet embedding (document.head || querySelector).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe PR fixes a crash when parsing HTML documents that don't have a Files Reviewed (1 file)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // detect used keyframes | ||
| if (decl.prop === 'animation' || decl.prop === 'animation-name') { | ||
| // Filter out common animation keywords and durations | ||
| const keywords = ['none', 'inherit', 'initial', 'unset', 'infinite', 'alternate', 'alternate-reverse', 'forwards', 'backwards', 'both', 'running', 'paused', 'normal', 'reverse', 'ease', 'ease-in', 'ease-out', 'ease-in-out', 'linear', 'step-start', 'step-end']; |
Contributor
There was a problem hiding this comment.
SUGGESTION: Consider moving the animation keywords array outside the loop or function to avoid recreating it on each call
| } | ||
| preload.setAttribute('crossorigin', 'anonymous'); | ||
| preload.setAttribute('href', fontUrl); | ||
| const head = document.head || document.querySelector('head'); |
Contributor
There was a problem hiding this comment.
SUGGESTION: Add a comment explaining the fallback for when document.head is null
| preload.setAttribute('href', src.trim()); | ||
| document.head.appendChild(preload); | ||
| if (src && shouldPreloadFonts) { | ||
| const urlRegex = /url\s*\(\s*(['"]?)(.+?)\1\s*\)(\s*format\s*\(\s*(['"]?)(.+?)\4\s*\))?/g; |
Contributor
There was a problem hiding this comment.
SUGGESTION: Add error handling for invalid font URLs or failed regex matches
Contributor
Code Review SummaryStatus: 3 Suggestions Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
|
Owner
Author
|
|
Owner
Author
|
Superseded by #59 (squash merge of all modernization PRs). Changes included in main. |
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.
Closes #26.
srcproperty.typeattribute to font preloads when format is specified.TODOindom.js.