Add Next.js 15 support #846
Conversation
|
WalkthroughThe pull request introduces a series of changes across multiple files, primarily focusing on enhancing the caching mechanisms and configuration for a cache-testing application. Key modifications include updates to workflow commands, the addition of new configuration files, new components for handling caching and revalidation, and enhancements to existing cache handling logic. New environment variables and types are defined to improve type safety, and a comprehensive suite of tests is introduced to validate cache behaviors. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (116)
apps/cache-testing-15-app/src/app/app/static/route.ts (1)
1-3: 🧹 Nitpick (assertive)
Add cache headers and type annotations for the static route.
The implementation is correct but could be enhanced for better caching and type safety.
Consider applying these improvements:
- export function GET() { + export async function GET(): Promise<Response> { - return Promise.resolve(new Response('OK', { status: 200 })); + return new Response('OK', { + status: 200, + headers: { + 'Cache-Control': 'public, max-age=31536000, immutable' + } + }); }This change:
- Adds explicit return type for better maintainability
- Sets appropriate cache headers for static content
- Simplifies the Promise handling since async functions automatically wrap returns
📝 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.export async function GET(): Promise<Response> { return new Response('OK', { status: 200, headers: { 'Cache-Control': 'public, max-age=31536000, immutable' } }); }apps/cache-testing-15-app/.env.example (3)
5-5:
⚠️ Potential issueWarning: Debug cache mode enabled by default.
The debug cache mode is enabled by default, which might impact performance. Consider disabling it by default and documenting its purpose.
-NEXT_PRIVATE_DEBUG_CACHE=1 +# Next.js cache debugging (0: disabled, 1: enabled) +# Warning: Enable only for development/debugging as it may impact performance +NEXT_PRIVATE_DEBUG_CACHE=0📝 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.# Next.js cache debugging (0: disabled, 1: enabled) # Warning: Enable only for development/debugging as it may impact performance NEXT_PRIVATE_DEBUG_CACHE=0
4-4: 🧹 Nitpick (assertive)
Document Partial Page Rendering (PPR) implications.
Since PPR is a new feature in Next.js 15, consider adding a comment explaining its purpose and impact on the application.
-PPR_ENABLED=false +# Partial Page Rendering (Next.js 15 feature) +# Enable to use streaming and partial rendering capabilities +# Note: Ensure your application is compatible before enabling in production +PPR_ENABLED=false📝 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.# Partial Page Rendering (Next.js 15 feature) # Enable to use streaming and partial rendering capabilities # Note: Ensure your application is compatible before enabling in production PPR_ENABLED=false
1-2: 🧹 Nitpick (assertive)
Add documentation for production deployment.
The Redis and cache server URLs are configured for local development. Consider adding comments to indicate the expected production values or configuration process.
-REDIS_URL=redis://localhost:6379 -REMOTE_CACHE_SERVER_BASE_URL=http://localhost:8080 +# Redis connection URL (local development) +# Production: Use secure Redis connection string from your provider +REDIS_URL=redis://localhost:6379 + +# Remote cache server URL (local development) +# Production: Use your deployed cache server URL with appropriate security +REMOTE_CACHE_SERVER_BASE_URL=http://localhost:8080📝 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.# Redis connection URL (local development) # Production: Use secure Redis connection string from your provider REDIS_URL=redis://localhost:6379 # Remote cache server URL (local development) # Production: Use your deployed cache server URL with appropriate security REMOTE_CACHE_SERVER_BASE_URL=http://localhost:8080apps/cache-testing-15-app/src/utils/normalize-slug.ts (1)
1-9: 🛠️ Refactor suggestion
Consider enhancing maintainability with these improvements.
- Add JSDoc documentation to explain the function's purpose and use cases
- Define string literal type for better type safety
- Extract magic strings as named constants
Here's a suggested implementation:
+/** Possible slug values for routing normalization */ +type RouteSlug = '404' | 'alternate-200-404' | '200'; + +const SLUGS = { + NOT_FOUND: '404', + ALTERNATE_NOT_FOUND: 'alternate-200-404', + OK: '200' +} as const; + +/** + * Normalizes route slugs for consistent handling of status codes + * @param slug - The input slug to normalize + * @returns Normalized slug representing the route status + */ -export function normalizeSlug(slug = ''): string { +export function normalizeSlug(slug = ''): RouteSlug { switch (slug) { - case '404': - case 'alternate-200-404': + case SLUGS.NOT_FOUND: + case SLUGS.ALTERNATE_NOT_FOUND: return slug; default: - return '200'; + return SLUGS.OK; } }📝 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./** Possible slug values for routing normalization */ type RouteSlug = '404' | 'alternate-200-404' | '200'; const SLUGS = { NOT_FOUND: '404', ALTERNATE_NOT_FOUND: 'alternate-200-404', OK: '200' } as const; /** * Normalizes route slugs for consistent handling of status codes * @param slug - The input slug to normalize * @returns Normalized slug representing the route status */ export function normalizeSlug(slug = ''): RouteSlug { switch (slug) { case SLUGS.NOT_FOUND: case SLUGS.ALTERNATE_NOT_FOUND: return slug; default: return SLUGS.OK; } }apps/cache-testing-15-app/.eslintrc.js (2)
6-8: 🛠️ Refactor suggestion
Consider optimizing TypeScript project resolution.
Setting
project: trueenables project-wide TypeScript checking, which might impact performance in larger codebases.Consider explicitly specifying the TypeScript config path for better performance:
parserOptions: { - project: true, + project: './tsconfig.json', },📝 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.parserOptions: { project: './tsconfig.json', },
9-9: 💡 Codebase verification
⚠️ Potential issueRemove the .mjs ignore pattern - these files need linting
The cache handler files contain critical logic including:
- Redis client configuration and error handling
- Environment variable validation
- Type annotations and JSDoc comments
- Complex async operations
These files would benefit from linting to catch potential issues with:
- TypeScript checks (they use
@ts-check)- Environment variable usage
- Async/await patterns
- Error handling
Suggested change:
- ignorePatterns: ['*.mjs'],🔗 Analysis chain
Reconsider ignoring .mjs files.
The configuration ignores all
.mjsfiles, but the AI summary mentions cache handlers with.mjsextension. This might lead to these important files being excluded from linting.Consider either:
- Removing the ignore pattern if
.mjsfiles should be linted- Moving specific files to the
overridessection if only certain.mjsfiles should be linted:- ignorePatterns: ['*.mjs'], + overrides: [ + { + files: ['**/cache-handler-*.mjs'], + rules: { + // Add specific rules for cache handlers + } + } + ],
This will help us understand:
- The complexity of the cache handler files and whether they would benefit from linting
- The current ESLint configuration context to make a more informed decision
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for .mjs files that might need linting # Test: Look for .mjs files in the project echo "Checking for .mjs files that might need linting..." fd -e mjs -x echo "Found .mjs file: {}"Length of output: 1108
Script:
#!/bin/bash # Let's check the content of cache handler files to understand their complexity and if they need linting echo "Checking content of cache handler files..." for file in $(fd "cache-handler.*\.mjs"); do echo "=== Content of $file ===" head -n 20 "$file" echo -e "\n" done # Let's also check the ESLint configuration to see if there are any specific rules that would be important for .mjs files echo "=== Checking ESLint configuration ===" cat apps/cache-testing-15-app/.eslintrc.jsLength of output: 5427
apps/cache-testing-15-app/src/utils/format-time.ts (2)
1-1: 🧹 Nitpick (assertive)
Add JSDoc documentation for better maintainability.
Consider adding documentation to describe the function's purpose, parameters, and return value.
+/** + * Formats a time value into a consistent string representation using en-GB locale. + * @param time - The time to format. Defaults to current time if not provided. + * @param fractionalSecondDigits - Optional precision for fractional seconds (1-3 digits). + * @returns Formatted time string in HH:mm:ss format with optional fractional seconds. + * @throws {Error} When provided with an invalid date input. + * @example + * formatTime() // Returns current time like "14:30:45" + * formatTime(date, 3) // Returns time with milliseconds like "14:30:45.123" + */ export function formatTime(time: number | string | Date = Date.now(), fractionalSecondDigits?: 1 | 2 | 3): string {📝 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./** * Formats a time value into a consistent string representation using en-GB locale. * @param time - The time to format. Defaults to current time if not provided. * @param fractionalSecondDigits - Optional precision for fractional seconds (1-3 digits). * @returns Formatted time string in HH:mm:ss format with optional fractional seconds. * @throws {Error} When provided with an invalid date input. * @example * formatTime() // Returns current time like "14:30:45" * formatTime(date, 3) // Returns time with milliseconds like "14:30:45.123" */ export function formatTime(time: number | string | Date = Date.now(), fractionalSecondDigits?: 1 | 2 | 3): string {
1-8: 🛠️ Refactor suggestion
Add input validation and error handling.
The current implementation silently handles invalid dates. Consider adding explicit error handling for better debugging.
export function formatTime(time: number | string | Date = Date.now(), fractionalSecondDigits?: 1 | 2 | 3): string { - return new Date(time).toLocaleTimeString('en-GB', { + const date = new Date(time); + if (isNaN(date.getTime())) { + throw new Error('Invalid date input provided to formatTime'); + } + return date.toLocaleTimeString('en-GB', { fractionalSecondDigits, hour: '2-digit', minute: '2-digit', second: '2-digit', }); }📝 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.export function formatTime(time: number | string | Date = Date.now(), fractionalSecondDigits?: 1 | 2 | 3): string { const date = new Date(time); if (isNaN(date.getTime())) { throw new Error('Invalid date input provided to formatTime'); } return date.toLocaleTimeString('en-GB', { fractionalSecondDigits, hour: '2-digit', minute: '2-digit', second: '2-digit', }); }apps/cache-testing-15-app/cache-handler-local.mjs (2)
1-4: 🛠️ Refactor suggestion
Consider converting to TypeScript for enhanced type safety.
While
@ts-checkprovides basic type checking, converting this file to TypeScript (.tsextension) would offer better type safety and IDE support, especially since this is part of the cache handling infrastructure.-// @ts-check +import type { CacheHandlerInstance } from '@neshca/cache-handler'; import { CacheHandler } from '@neshca/cache-handler'; import createLruHandler from '@neshca/cache-handler/local-lru';Committable suggestion was skipped due to low confidence.
6-12: 🛠️ Refactor suggestion
Add error handling and configuration options.
The cache handler initialization could benefit from:
- Error handling for handler creation
- Configurable LRU options (e.g., max size, TTL)
- Documentation about the handler's behavior
-CacheHandler.onCreation(() => { - const localHandler = createLruHandler(); +/** + * Initializes a local LRU cache handler with default configuration. + * @throws {Error} If handler creation fails + */ +CacheHandler.onCreation(() => { + try { + const localHandler = createLruHandler({ + // Consider making these configurable via environment variables + maxSize: 100 * 1024 * 1024, // 100MB + ttl: 60 * 60 * 1000, // 1 hour + }); - return { - handlers: [localHandler], - }; + return { + handlers: [localHandler], + }; + } catch (error) { + console.error('Failed to initialize local cache handler:', error); + throw error; + } });Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/src/instrumentation.ts (1)
1-7: 🧹 Nitpick (assertive)
Consider adding error handling and type safety improvements.
The implementation correctly follows Next.js instrumentation patterns and properly handles async operations. However, consider these improvements:
- Add error handling to gracefully handle import or registration failures
- Add explicit type annotations for better type safety
- Consider using module aliases instead of relative imports
Here's a suggested implementation with the improvements:
-export async function register() { +export async function register(): Promise<void> { if (process.env.NEXT_RUNTIME === 'nodejs') { - const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); - const CacheHandler = (await import('../cache-handler-redis-stack.mjs')).default; - await registerInitialCache(CacheHandler); + try { + const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); + const { default: CacheHandler } = await import('../cache-handler-redis-stack.mjs'); + await registerInitialCache(CacheHandler); + } catch (error) { + console.error('Failed to initialize cache handler:', error); + // Re-throw to ensure Next.js is aware of the initialization failure + throw error; + } } }📝 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.export async function register(): Promise<void> { if (process.env.NEXT_RUNTIME === 'nodejs') { try { const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); const { default: CacheHandler } = await import('../cache-handler-redis-stack.mjs'); await registerInitialCache(CacheHandler); } catch (error) { console.error('Failed to initialize cache handler:', error); // Re-throw to ensure Next.js is aware of the initialization failure throw error; } } }internal/next-common/package.json (1)
11-11:
⚠️ Potential issueVersion inconsistency with related packages.
The AI summary indicates that related packages (
@repo/cache-testingand@repo/cache-handler-docs) are using Next.js 14.2.16, while this package is being upgraded to 15.0.0. This version mismatch could lead to compatibility issues.Consider either:
- Upgrading all related packages to Next.js 15.0.0 for consistency
- Or keeping this package at 14.2.16 to maintain version alignment
apps/cache-testing-15-app/src/components/pre-rendered-at.tsx (2)
5-5: 🧹 Nitpick (assertive)
Consider renaming the type and adding documentation.
The type name
CacheStateWatcherPropsseems to be a misnomer as it's used for a component that displays pre-render time, not watching cache state.Consider this improvement:
-type CacheStateWatcherProps = { time: number; isFallback?: boolean }; +/** + * Props for the PreRenderedAt component + * @property time - Timestamp of when the page was pre-rendered + * @property isFallback - Whether the page is in fallback state + */ +type PreRenderedAtProps = { time: number; isFallback?: boolean };📝 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./** * Props for the PreRenderedAt component * @property time - Timestamp of when the page was pre-rendered * @property isFallback - Whether the page is in fallback state */ type PreRenderedAtProps = { time: number; isFallback?: boolean };
7-11: 🧹 Nitpick (assertive)
Consider enhancing accessibility while maintaining current functionality.
The component logic is sound, but we could improve its semantic meaning and accessibility.
Consider this enhancement:
export function PreRenderedAt({ time, isFallback }: CacheStateWatcherProps): JSX.Element { const preRenderTime = isFallback ? '' : formatTime(time, 3); - return <div data-pw="pre-rendered-at">Pre-rendered at {preRenderTime}</div>; + return ( + <p + data-pw="pre-rendered-at" + aria-label={`Page pre-rendered at ${preRenderTime}`} + > + Pre-rendered at {preRenderTime} + </p> + ); }This change:
- Uses semantic
<p>tag for text content- Adds aria-label for better screen reader support
- Maintains existing functionality and test hooks
📝 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.export function PreRenderedAt({ time, isFallback }: CacheStateWatcherProps): JSX.Element { const preRenderTime = isFallback ? '' : formatTime(time, 3); return ( <p data-pw="pre-rendered-at" aria-label={`Page pre-rendered at ${preRenderTime}`} > Pre-rendered at {preRenderTime} </p> ); }apps/cache-testing-15-app/cluster.config.js (3)
6-7: 🛠️ Refactor suggestion
Consider enhancing the production configuration
The current configuration could be improved for production readiness:
- The number of instances is hardcoded to 2
- No restart strategy or error handling is defined
- Using 'localhost' as hostname in production is not recommended
Consider applying these production-ready configurations:
name: 'app', script: '.next/__instances/3000/server.js', - instances: 2, + instances: 'max', // Uses number of available CPU cores exec_mode: 'cluster', + max_memory_restart: '1G', + restart_delay: 3000, + exp_backoff_restart_delay: 100, env_production: { NODE_ENV: 'production', - HOSTNAME: 'localhost', + HOSTNAME: process.env.HOSTNAME || '0.0.0.0', REDIS_URL: 'redis://localhost:6379', SERVER_STARTED: '1', },Also applies to: 8-13
1-16: 🛠️ Refactor suggestion
Consider adding TypeScript support
Since the PR mentions removing
@ts-expect-error, consider converting this configuration to TypeScript for better type safety and consistency with the rest of the codebase.Create a new file
cluster.config.ts:import { type Config } from 'pm2'; const config: Config = { apps: [ { name: 'app', script: '.next/__instances/3000/server.js', instances: 'max', exec_mode: 'cluster', max_memory_restart: '1G', restart_delay: 3000, exp_backoff_restart_delay: 100, env_production: { NODE_ENV: 'production', HOSTNAME: process.env.HOSTNAME || '0.0.0.0', REDIS_URL: process.env.REDIS_URL || 'redis://:password@localhost:6379', SERVER_STARTED: '1', }, }, ], }; export default config;
11-11:
⚠️ Potential issueSecurity: Redis connection URL should not use default configuration in production
The Redis connection URL is set to connect without authentication on the default port. In a production environment, this could pose a security risk.
Consider:
- Using authentication for Redis
- Making the Redis URL configurable through environment variables
- REDIS_URL: 'redis://localhost:6379', + REDIS_URL: process.env.REDIS_URL || 'redis://:password@localhost:6379',📝 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.REDIS_URL: process.env.REDIS_URL || 'redis://:password@localhost:6379',apps/cache-testing-15-app/tsconfig.json (3)
12-12: 🧹 Nitpick (assertive)
Excellent: Strict null checks enabled.
This aligns perfectly with the PR objective of removing
@ts-expect-errorinstances and improving type safety. Consider enabling additional strict checks for even better type safety.-"strictNullChecks": true +"strict": true, +"noUncheckedIndexedAccess": true📝 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."strict": true, "noUncheckedIndexedAccess": true
4-6: 🛠️ Refactor suggestion
Consider using more specific path aliases.
While the current path mapping works, consider adding more granular aliases for commonly accessed directories (e.g., components, hooks, utils) to improve code organization and maintainability.
"paths": { - "cache-testing-15-app/*": ["./src/*"] + "cache-testing-15-app/*": ["./src/*"], + "@components/*": ["./src/components/*"], + "@hooks/*": ["./src/hooks/*"], + "@utils/*": ["./src/utils/*"] }Committable suggestion was skipped due to low confidence.
14-15: 💡 Codebase verification
Next.js 15 type definitions setup needs attention
The tsconfig.json includes are correct, but the project is missing the required
next-env.d.tsfile which is essential for Next.js 15 TypeScript support. This file should be auto-generated when running Next.js for the first time.
- Create
next-env.d.tsby runningnextonce locally or add it manually with the following content:/// <reference types="next" /> /// <reference types="next/image-types/global" />🔗 Analysis chain
Verify Next.js 15 type definitions.
The includes look good, but let's verify that all necessary Next.js 15 type definitions are properly included.
Let me gather more information about the Next.js setup in this repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Next.js 15 type definitions are present and properly referenced # Check if next-env.d.ts exists and contains Next.js 15 references echo "Checking next-env.d.ts..." cat next-env.d.ts 2>/dev/null || echo "Warning: next-env.d.ts not found" # Check for Next.js version in package.json echo "Checking Next.js version..." jq '.dependencies.next, .devDependencies.next' package.json 2>/dev/null || echo "Warning: Unable to determine Next.js version" # Verify .next/types exists after build echo "Checking for generated types..." ls -la .next/types 2>/dev/null || echo "Note: .next/types will be generated during build"Length of output: 493
Script:
#!/bin/bash # Check package.json in the app directory echo "Checking app's package.json..." cat apps/cache-testing-15-app/package.json # Look for any Next.js related configuration files echo -e "\nChecking for Next.js config files..." fd -t f "next.config" apps/cache-testing-15-app # Check if there are any TypeScript files that might need Next.js types echo -e "\nChecking for TypeScript files..." fd -e ts -e tsx . apps/cache-testing-15-app --exclude node_modulesLength of output: 3637
apps/cache-testing-15-app/src/app/app/no-params/ssr/200/page.tsx (1)
8-8: 🧹 Nitpick (assertive)
Consider adding type annotations for getData function.
While the code works, adding explicit type annotations would improve type safety and documentation.
-const getData = createGetData('app/no-params/ssr', undefined, 'no-store'); +const getData: (status: string) => Promise<{ count: number; path: string; time: number; } | undefined> = + createGetData('app/no-params/ssr', undefined, 'no-store');📝 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 getData: (status: string) => Promise<{ count: number; path: string; time: number; } | undefined> = createGetData('app/no-params/ssr', undefined, 'no-store');apps/cache-testing-15-app/src/components/restart-button.tsx (4)
1-3: 🧹 Nitpick (assertive)
Consider adding error boundary
Since this is a client component handling API calls, it would be beneficial to wrap it in an error boundary to gracefully handle runtime errors.
Would you like me to provide an example implementation of an error boundary component?
19-24:
⚠️ Potential issueEnhance button accessibility and user experience
The button lacks proper accessibility attributes and loading state handling.
Apply these improvements:
<div> <button data-pw="restart-button" onClick={restart} type="button" + disabled={isLoading} + aria-busy={isLoading} + aria-label="Restart application" > - Restart app + {isLoading ? 'Restarting...' : 'Restart app'} </button> - <div data-pw="restart-state">{restartState}</div> + <div + data-pw="restart-state" + role="status" + aria-live="polite" + > + {restartState} + </div> </div>Committable suggestion was skipped due to low confidence.
6-6:
⚠️ Potential issueAdd loading state management
Add a loading state to properly handle the button's disabled state during restart operation.
-const [restartState, setRestartState] = useState(''); +const [restartState, setRestartState] = useState(''); +const [isLoading, setIsLoading] = useState(false);📝 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 [restartState, setRestartState] = useState(''); const [isLoading, setIsLoading] = useState(false);
8-16:
⚠️ Potential issueFix race condition in state management and improve error handling
The current implementation has several issues:
- Race condition: Setting
restartStateafter the fetch call starts means the error state might overwrite the "Restarting..." state- No error details are captured from the API response
- No proper error handling for network failures
Consider this improved implementation:
function restart(): void { + setRestartState('Restarting...'); + setIsLoading(true); fetch('/api/restart-app').then((result) => { - if (!result.ok) { - setRestartState('Fail to restart'); - } + return result.json().catch(() => { + throw new Error(`HTTP error! status: ${result.status}`); + }); + }).catch((error) => { + setRestartState(`Failed to restart: ${error.message}`); + }).finally(() => { + setIsLoading(false); }); - - setRestartState('Restarting...'); }Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/src/utils/common-app-page.tsx (2)
10-13: 🛠️ Refactor suggestion
Enhance accessibility with semantic HTML and ARIA attributes.
The current implementation uses generic
divelements. Consider using semantic HTML elements and adding appropriate ARIA attributes for better accessibility.- <div> - <div data-pw="data" id={path}> + <main> + <section + data-pw="data" + id={path} + aria-label="Page Content" + role="region"> {count} - </div> + </section>📝 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.<main> <section data-pw="data" id={path} aria-label="Page Content" role="region"> {count} </section>
8-20: 🧹 Nitpick (assertive)
Add JSDoc documentation for the component.
Consider adding documentation to describe the component's purpose, props, and usage examples.
+/** + * CommonAppPage is a shared component for rendering page content with cache state monitoring. + * @param {Object} props - Component props + * @param {number} props.count - The count value to display + * @param {number} props.revalidateAfter - Time in seconds after which the cache should revalidate + * @param {string} props.time - Timestamp for pre-rendering information + * @param {string} props.path - Unique path identifier for the page + * @returns {JSX.Element} Rendered page content with cache monitoring + */ export function CommonAppPage({ count, revalidateAfter, time, path }: PageProps): JSX.Element {📝 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./** * CommonAppPage is a shared component for rendering page content with cache state monitoring. * @param {Object} props - Component props * @param {number} props.count - The count value to display * @param {number} props.revalidateAfter - Time in seconds after which the cache should revalidate * @param {string} props.time - Timestamp for pre-rendering information * @param {string} props.path - Unique path identifier for the page * @returns {JSX.Element} Rendered page content with cache monitoring */ export function CommonAppPage({ count, revalidateAfter, time, path }: PageProps): JSX.Element { return ( <div> <div data-pw="data" id={path}> {count} </div> <PreRenderedAt time={time} /> <Suspense fallback={null}> <CacheStateWatcher revalidateAfter={revalidateAfter} time={time} /> </Suspense> </div> ); }apps/cache-testing-15-app/playwright.config.ts (1)
5-12: 🛠️ Refactor suggestion
Consider enabling retries in CI environment.
While the current configuration (no retries, single worker, sequential execution) is good for deterministic cache testing, having zero retries (
retries: 0) might lead to flaky CI builds. Consider enabling at least one retry for CI environments:- retries: 0, + retries: process.env.CI ? 1 : 0,The rest of the configuration is well-thought-out:
- Sequential execution prevents cache race conditions
- Single worker ensures predictable test order
- HTML reporter helps with debugging failed tests
📝 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.export default defineConfig({ testDir: './tests', fullyParallel: false, forbidOnly: Boolean(process.env.CI), retries: process.env.CI ? 1 : 0, workers: 1, reporter: 'html', use: { baseURL: 'http://localhost', trace: 'on-first-retry', testIdAttribute: 'data-pw' },apps/cache-testing-15-app/src/app/app/no-params/dynamic-true/[slug]/page.tsx (1)
16-27: 🛠️ Refactor suggestion
Consider enhancing error handling.
While the implementation is solid, consider adding error boundaries or try-catch blocks to handle potential errors during data fetching and processing. This would provide a better user experience when network issues occur.
Here's a suggested enhancement:
export default async function Index(props: PageParams): Promise<JSX.Element> { + try { const params = await props.params; const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; + } catch (error) { + console.error('Error fetching data:', error); + throw error; // Let Next.js error boundary handle it + } }📝 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.export default async function Index(props: PageParams): Promise<JSX.Element> { try { const params = await props.params; const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; } catch (error) { console.error('Error fetching data:', error); throw error; // Let Next.js error boundary handle it } }apps/cache-testing-15-app/src/app/app/no-params/dynamic-false/[slug]/page.tsx (2)
12-12:
⚠️ Potential issueReview the Promise wrapper in PageParams type.
The
paramsproperty being wrapped in a Promise is unusual for Next.js route parameters. Next.js typically provides route parameters directly as values, not as Promises.Consider updating the type definition:
-type PageParams = { params: Promise<{ slug: string }> }; +type PageParams = { params: { slug: string } };📝 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.type PageParams = { params: { slug: string } };
16-27: 🧹 Nitpick (assertive)
LGTM! Well-structured component with proper error handling.
The implementation follows Next.js best practices with appropriate error handling and data passing.
Consider adding type safety for the destructured data:
- const { count, path, time } = data; + const { count, path, time }: { count: number; path: string; time: number } = data;📝 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.export default async function Index(props: PageParams): Promise<JSX.Element> { const params = await props.params; const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time }: { count: number; path: string; time: number } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; }docs/cache-handler-docs/package.json (1)
14-14: 💡 Codebase verification
Next.js version discrepancy requires attention
The codebase shows mixed usage of Next.js versions:
- Next.js 15.0.0:
@repo/next-common,@repo/cache-testing-15-app- Next.js 14.2.16:
@repo/cache-handler-docs,@repo/cache-testingThis mixed version state suggests that the PR's Next.js 15 support is being implemented gradually, with some packages already upgraded while others remain on 14.2.16. The docs package should also be upgraded to version 15 for consistency.
🔗 Analysis chain
Version inconsistency with PR objective
The PR description mentions adding Next.js 15 support, but the changes show an update to version 14.2.16. This seems inconsistent with the stated objective.
Let's verify the Next.js versions across the project:
Also applies to: 21-21
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Next.js versions across all package.json files # Expected: Consistent Next.js 15.x versions if PR objective is accurate echo "Checking Next.js versions across package.json files:" fd "package.json" --exec jq -r 'select(.dependencies.next) | "\(.name): Next.js \(.dependencies.next)"'Length of output: 374
apps/cache-testing-15-app/src/app/layout.tsx (3)
18-22: 🛠️ Refactor suggestion
Consider adding error boundaries for better error handling.
The Suspense-wrapped components could benefit from error boundary implementation to gracefully handle potential failures in the revalidation or restart operations.
// Create an ErrorBoundary component class ErrorBoundary extends React.Component<PropsWithChildren> { state = { hasError: false }; static getDerivedStateFromError() { return { hasError: true }; } render() { if (this.state.hasError) { return <div>Something went wrong with the controls.</div>; } return this.props.children; } } // Use it in the layout <ErrorBoundary> <Suspense fallback={null}> <RevalidateButton nextApi="app" type="path" /> <RevalidateButton nextApi="app" type="tag" /> <RestartButton /> </Suspense> </ErrorBoundary>
8-11: 🧹 Nitpick (assertive)
Add a meaningful description for SEO optimization.
The metadata description is currently empty. Consider adding a descriptive summary of the application to improve SEO.
export const metadata = { title: 'Cache testing app', - description: '', + description: 'Next.js 15 cache testing application for validating caching mechanisms and revalidation strategies', };📝 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.export const metadata = { title: 'Cache testing app', description: 'Next.js 15 cache testing application for validating caching mechanisms and revalidation strategies', };
15-16: 🧹 Nitpick (assertive)
Add viewport meta tag for responsive design.
Consider adding a viewport meta tag to ensure proper responsive behavior across different devices.
<html lang="en"> + <head> + <meta name="viewport" content="width=device-width, initial-scale=1.0" /> + </head> <body>Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/src/app/app/ppr/page.tsx (2)
23-25: 🧹 Nitpick (assertive)
Consider enhancing the skeleton UI
The current skeleton is a simple text placeholder. Consider using a more visually representative loading state that matches the actual content's layout and dimensions.
Here's a suggestion for a more realistic skeleton:
function Skeleton(): JSX.Element { - return <div data-pw="ppr-prerendered">Skeleton</div>; + return ( + <div data-pw="ppr-prerendered" className="animate-pulse"> + <div className="h-6 w-32 bg-gray-200 rounded" /> + </div> + ); }📝 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.function Skeleton(): JSX.Element { return ( <div data-pw="ppr-prerendered" className="animate-pulse"> <div className="h-6 w-32 bg-gray-200 rounded" /> </div> ); }
7-21: 🛠️ Refactor suggestion
Enhance error handling and configuration
A few suggestions to improve the robustness of this component:
- The error message could include more context (e.g., HTTP status code)
- The API URL should be configurable via environment variables
- The cache tag could be defined as a constant to ensure consistency
Consider applying these improvements:
+const API_URL = process.env.NEXT_PUBLIC_TIME_API_URL || 'http://localhost:8081'; +const CACHE_TAG = '/app/ppr'; async function ActualData(): Promise<JSX.Element> { noStore(); - const response = await fetch('http://localhost:8081/time', { - next: { tags: ['/app/ppr'] }, + const response = await fetch(`${API_URL}/time`, { + next: { tags: [CACHE_TAG] }, }); if (!response.ok) { - throw new Error('Failed to fetch unix time'); + throw new Error(`Failed to fetch unix time: ${response.status} ${response.statusText}`); }📝 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 API_URL = process.env.NEXT_PUBLIC_TIME_API_URL || 'http://localhost:8081'; const CACHE_TAG = '/app/ppr'; async function ActualData(): Promise<JSX.Element> { noStore(); const response = await fetch(`${API_URL}/time`, { next: { tags: [CACHE_TAG] }, }); if (!response.ok) { throw new Error(`Failed to fetch unix time: ${response.status} ${response.statusText}`); } const data = (await response.json()) as TimeBackendApiResponseJson; return <div data-pw="ppr-postponed">{formatTime(data.unixTimeMs)}</div>; }apps/cache-testing-15-app/src/app/app/with-params/dynamic-true/[slug]/page.tsx (1)
16-22: 🛠️ Refactor suggestion
Simplify the generateStaticParams implementation
The current implementation unnecessarily wraps the params in multiple Promises. This can be simplified for better readability and maintenance.
Consider simplifying the implementation:
-export function generateStaticParams(): Promise<PageParams['params'][]> { - return Promise.resolve([ - Promise.resolve({ slug: '200' }), - Promise.resolve({ slug: '404' }), - Promise.resolve({ slug: 'alternate-200-404' }), - ]); -} +export function generateStaticParams(): Promise<{ slug: string }[]> { + return Promise.resolve([ + { slug: '200' }, + { slug: '404' }, + { slug: 'alternate-200-404' }, + ]); +}📝 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.export function generateStaticParams(): Promise<{ slug: string }[]> { return Promise.resolve([ { slug: '200' }, { slug: '404' }, { slug: 'alternate-200-404' }, ]); }apps/cache-testing-15-app/cache-handler-next-example.js (6)
3-6: 🧹 Nitpick (assertive)
Document and validate constructor options.
The
optionsparameter lacks validation and documentation. Consider:
- Adding JSDoc or TypeScript types
- Validating required options
- Documenting expected options shape
Example improvement:
/** * @typedef {Object} CacheHandlerOptions * @property {number} [maxSize] - Maximum number of entries (default: 100) * @property {number} [ttl] - Time-to-live in milliseconds (default: 3600000) */ module.exports = class CacheHandler { /** * @param {CacheHandlerOptions} options */ constructor(options = {}) { this.options = { maxSize: 100, ttl: 3600000, ...options }; } }
1-35: 🧹 Nitpick (assertive)
Consider enhancing the cache implementation with production-ready features.
For a production-ready Next.js 15 cache handler, consider implementing:
Durable Storage Integration:
- Add Redis/Memcached adapter support
- Implement fallback mechanisms
- Add persistence options
Monitoring & Debugging:
- Add cache hit/miss metrics
- Implement debug logging
- Add performance tracking
Advanced Features:
- Implement stale-while-revalidate
- Add cache warming capabilities
- Support distributed cache invalidation
Would you like assistance in implementing any of these features?
14-22:
⚠️ Potential issueAdd input validation and size management to set method.
The current implementation lacks:
- Input parameter validation
- Cache size management
- Error handling for cache operations
Apply this diff:
async set(key, data, ctx) { - // This could be stored anywhere, like durable storage - cache.set(key, { - value: data, - lastModified: Date.now(), - tags: ctx.tags, - }); + try { + if (!key) throw new Error('Cache key is required'); + if (!ctx?.tags || !Array.isArray(ctx.tags)) { + throw new Error('Tags must be an array'); + } + + // Enforce size limit + if (this.options.maxSize && this.cache.size >= this.options.maxSize) { + const oldestKey = this.cache.keys().next().value; + this.cache.delete(oldestKey); + } + + this.cache.set(key, { + value: data, + lastModified: Date.now(), + tags: ctx.tags, + }); + } catch (error) { + console.error('Cache set operation failed:', error); + throw error; + } }Committable suggestion was skipped due to low confidence.
24-34: 🛠️ Refactor suggestion
Optimize tag revalidation performance and add error handling.
Current implementation has performance and reliability concerns:
- O(n) complexity for tag revalidation
- Missing error handling
- Unnecessary use of
letin loopConsider maintaining a tag-to-key index for O(1) lookups. Apply this diff:
+ constructor(options) { + this.options = options; + this.cache = new Map(); + this.tagIndex = new Map(); + } + #indexTag(key, tags) { + for (const tag of tags) { + if (!this.tagIndex.has(tag)) { + this.tagIndex.set(tag, new Set()); + } + this.tagIndex.get(tag).add(key); + } + } async revalidateTag(tag) { - // Iterate over all entries in the cache - // biome-ignore lint/style/useConst: don't bother - for (let [key, value] of cache) { - // If the value's tags include the specified tag, delete this entry - if (value.tags.includes(tag)) { - cache.delete(key); + try { + const keysToDelete = this.tagIndex.get(tag); + if (!keysToDelete) return; + + for (const key of keysToDelete) { + this.cache.delete(key); } + this.tagIndex.delete(tag); + } catch (error) { + console.error('Cache revalidation failed:', error); + throw error; } }Committable suggestion was skipped due to low confidence.
1-1:
⚠️ Potential issueMove cache instance to class level to prevent shared state issues.
The global
Mapinstance creates a shared singleton that could lead to memory leaks and race conditions in Next.js, especially when running multiple instances. Consider:
- Moving the cache to instance level
- Implementing size limits and eviction policies
Apply this diff:
-const cache = new Map(); module.exports = class CacheHandler { constructor(options) { this.options = options; + this.cache = new Map(); }Committable suggestion was skipped due to low confidence.
8-12:
⚠️ Potential issueEnhance get method with error handling and TTL checks.
The current implementation lacks:
- Error handling for cache operations
- TTL expiration checks
- Type validation of cached values
Apply this diff:
async get(key) { - // This could be stored anywhere, like durable storage - return cache.get(key); + try { + const entry = this.cache.get(key); + if (!entry) return undefined; + + // Check TTL + if (this.options.ttl && Date.now() - entry.lastModified > this.options.ttl) { + this.cache.delete(key); + return undefined; + } + + return entry.value; + } catch (error) { + console.error('Cache get operation failed:', error); + return undefined; + } }Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/src/app/app/with-params/dynamic-false/[slug]/page.tsx (3)
10-12: 🧹 Nitpick (assertive)
Consider adjusting the revalidation time.
A 5-second revalidation time is quite aggressive and might lead to unnecessary server load. Consider increasing this value unless there's a specific requirement for such frequent updates.
16-22: 🛠️ Refactor suggestion
Consider making static paths configurable.
The static paths are currently hardcoded. Consider moving these values to a configuration file for better maintainability and flexibility.
Example implementation:
// config/static-paths.ts export const STATIC_PATHS = ['200', '404', 'alternate-200-404']; // page.tsx export function generateStaticParams(): Promise<PageParams['params'][]> { return Promise.resolve( STATIC_PATHS.map(slug => Promise.resolve({ slug })) ); }
24-35: 🧹 Nitpick (assertive)
LGTM! Consider making the time conversion more explicit.
The implementation follows Next.js best practices with proper error handling and data flow. However, the revalidation time conversion could be made more explicit for better readability.
Consider this minor improvement:
- return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; + const revalidateAfterMs = revalidate * 1000; // Convert seconds to milliseconds + return <CommonAppPage count={count} path={path} revalidateAfter={revalidateAfterMs} time={time} />;📝 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.export default async function Index(props: PageParams): Promise<JSX.Element> { const params = await props.params; const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time } = data; const revalidateAfterMs = revalidate * 1000; // Convert seconds to milliseconds return <CommonAppPage count={count} path={path} revalidateAfter={revalidateAfterMs} time={time} />; }apps/cache-testing-15-app/src/app/app/with-params/nesh-cache/[slug]/page.tsx (3)
8-8:
⚠️ Potential issueSimplify the PageParams type definition
The
PageParamstype wraps the params object in a Promise, which is unnecessary as Next.js route parameters are always synchronously available.Simplify the type definition:
-type PageParams = { params: Promise<{ slug: string }> }; +type PageParams = { params: { slug: string } };📝 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.type PageParams = { params: { slug: string } };
16-22:
⚠️ Potential issueSimplify generateStaticParams implementation
The function unnecessarily wraps each parameter object in Promises.
Simplify the implementation:
-export function generateStaticParams(): Promise<PageParams['params'][]> { +export function generateStaticParams() { return Promise.resolve([ - Promise.resolve({ slug: '200' }), - Promise.resolve({ slug: '404' }), - Promise.resolve({ slug: 'alternate-200-404' }), + { slug: '200' }, + { slug: '404' }, + { slug: 'alternate-200-404' }, ]); }📝 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.export function generateStaticParams() { return Promise.resolve([ { slug: '200' }, { slug: '404' }, { slug: 'alternate-200-404' }, ]); }
24-35:
⚠️ Potential issueSimplify page component implementation
The component unnecessarily awaits the params object.
Simplify the implementation:
export default async function Index(props: PageParams): Promise<JSX.Element> { - const params = await props.params; - const data = await getData(params.slug); + const data = await getData(props.params.slug); if (!data) { notFound(); }📝 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.export default async function Index(props: PageParams): Promise<JSX.Element> { const data = await getData(props.params.slug); if (!data) { notFound(); } const { count, path, time } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; }apps/cache-testing-15-app/src/app/app/with-params/unstable-cache/[slug]/page.tsx (2)
16-22: 🛠️ Refactor suggestion
Simplify static params generation and consider configuration
The current implementation has unnecessary Promise wrapping and hardcoded values.
Consider simplifying and making it more configurable:
-export function generateStaticParams(): Promise<PageParams['params'][]> { - return Promise.resolve([ - Promise.resolve({ slug: '200' }), - Promise.resolve({ slug: '404' }), - Promise.resolve({ slug: 'alternate-200-404' }), - ]); -} +export function generateStaticParams(): { slug: string }[] { + return [ + { slug: '200' }, + { slug: '404' }, + { slug: 'alternate-200-404' } + ]; +}📝 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.export function generateStaticParams(): { slug: string }[] { return [ { slug: '200' }, { slug: '404' }, { slug: 'alternate-200-404' } ]; }
24-35: 🛠️ Refactor suggestion
Simplify async handling in the page component
The component has unnecessary async complexity due to the Promise-wrapped params type.
Consider simplifying:
-export default async function Index(props: PageParams): Promise<JSX.Element> { - const params = await props.params; +export default async function Index({ params }: { params: { slug: string } }): Promise<JSX.Element> { const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; }📝 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.export default async function Index({ params }: { params: { slug: string } }): Promise<JSX.Element> { const data = await getData(params.slug); if (!data) { notFound(); } const { count, path, time } = data; return <CommonAppPage count={count} path={path} revalidateAfter={revalidate * 1000} time={time} />; }apps/cache-testing-15-app/src/app/api/revalidate-app/route.ts (5)
5-5: 🧹 Nitpick (assertive)
Consider using a more robust import path.
The current import uses a package-style path (
cache-testing-15-app/utils/format-time). Consider using a relative path (e.g.,../../utils/format-time) or configuring proper path aliases in your TypeScript/Next.js configuration for better maintainability.
33-40:
⚠️ Potential issueImprove error handling and HTTP status codes.
The current implementation returns a 200 status code even when the request is invalid. Consider:
- Using appropriate HTTP status codes (e.g., 400 for invalid requests)
- Adding try/catch blocks to handle potential revalidation failures
- return Promise.resolve( - NextResponse.json({ + return NextResponse.json( + { revalidated: false, now: time, message: 'Missing path to revalidate', - }), - ); + }, + { status: 400 } + );Also consider adding error handling:
try { if (path) { revalidatePath(path); return createSuccessResponse(time); } // ... rest of the code } catch (error) { return NextResponse.json( { revalidated: false, now: time, message: 'Revalidation failed', error: error instanceof Error ? error.message : 'Unknown error', }, { status: 500 } ); }
7-10: 🛠️ Refactor suggestion
Simplify using async/await syntax.
The function returns wrapped promises but could be simplified using async/await syntax for better readability and maintainability.
-export function GET(request: NextRequest): Promise<NextResponse> { +export async function GET(request: NextRequest): Promise<NextResponse> {📝 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.export async function GET(request: NextRequest): Promise<NextResponse> { const path = request.nextUrl.searchParams.get('path'); const tag = request.nextUrl.searchParams.get('tag');
13-31: 🛠️ Refactor suggestion
Reduce code duplication in success responses.
Both revalidation paths return identical success responses. Consider extracting this to a helper function.
+const createSuccessResponse = (time: string) => + NextResponse.json({ + revalidated: true, + now: time, + }); + if (path) { revalidatePath(path); - return Promise.resolve( - NextResponse.json({ - revalidated: true, - now: time, - }), - ); + return createSuccessResponse(time); } if (tag) { revalidateTag(tag); - return Promise.resolve( - NextResponse.json({ - revalidated: true, - now: time, - }), - ); + return createSuccessResponse(time); }📝 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 createSuccessResponse = (time: string) => NextResponse.json({ revalidated: true, now: time, }); if (path) { revalidatePath(path); return createSuccessResponse(time); } if (tag) { revalidateTag(tag); return createSuccessResponse(time); }
11-11: 🧹 Nitpick (assertive)
Consider extracting the precision value as a named constant.
The magic number
3for time precision could be more meaningful as a named constant at the file level.+const TIME_PRECISION = 3; + export async function GET(request: NextRequest): Promise<NextResponse> { // ... - const time = formatTime(Date.now(), 3); + const time = formatTime(Date.now(), TIME_PRECISION);Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/create-instances.sh (5)
24-26:
⚠️ Potential issueFix unsafe glob pattern and add error handling
The current implementation has potential issues with globbing and doesn't handle empty directories properly.
# Copy fetch cache directory to standalone app/.next directory mkdir -p "$APP_DIR/.next/cache/fetch-cache/" -cp $FETCH_CACHE_DIR/* $APP_DIR/.next/cache/fetch-cache/ +if [ -n "$(ls -A "$FETCH_CACHE_DIR")" ]; then + cp -r "$FETCH_CACHE_DIR"/* "$APP_DIR/.next/cache/fetch-cache/" +else + echo "Warning: Fetch cache directory is empty" +fi📝 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.# Copy fetch cache directory to standalone app/.next directory mkdir -p "$APP_DIR/.next/cache/fetch-cache/" if [ -n "$(ls -A "$FETCH_CACHE_DIR")" ]; then cp -r "$FETCH_CACHE_DIR"/* "$APP_DIR/.next/cache/fetch-cache/" else echo "Warning: Fetch cache directory is empty" fi🧰 Tools
🪛 Shellcheck
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
35-41: 🛠️ Refactor suggestion
Improve instance creation scalability and efficiency
Consider making the script more flexible and efficient by parameterizing instance ports and adding parallel processing.
+# Clean up existing instances +if [ -d "$INSTANCES_DIR" ]; then + rm -rf "$INSTANCES_DIR" +fi + +# Define instance ports +INSTANCE_PORTS=(3000 3001) + # Create instance directories -create_instance_dir 3000 -create_instance_dir 3001 +for port in "${INSTANCE_PORTS[@]}"; do + create_instance_dir "$port" + # Use background processes for parallel copying + copy_dir "$STANDALONE_DIR/." "$INSTANCES_DIR/$port" & +done -# Copy files from standalone directory to instance directories -copy_dir "$STANDALONE_DIR/." "$INSTANCES_DIR/3000" -copy_dir "$STANDALONE_DIR/." "$INSTANCES_DIR/3001" +# Wait for all background processes to complete +wait📝 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.# Clean up existing instances if [ -d "$INSTANCES_DIR" ]; then rm -rf "$INSTANCES_DIR" fi # Define instance ports INSTANCE_PORTS=(3000 3001) # Create instance directories for port in "${INSTANCE_PORTS[@]}"; do create_instance_dir "$port" # Use background processes for parallel copying copy_dir "$STANDALONE_DIR/." "$INSTANCES_DIR/$port" & done # Wait for all background processes to complete wait
11-16: 🛠️ Refactor suggestion
Enhance copy_dir function with additional checks
The function should validate source directory existence and handle existing destination directories safely.
copy_dir() { + if [ ! -d "$1" ]; then + echo "Error: Source directory $1 does not exist" + exit 1 + fi + + if [ -e "$2" ]; then + echo "Warning: Destination $2 already exists, removing..." + rm -rf "$2" + fi + if ! cp -r "$1" "$2"; then echo "Failed to copy from $1 to $2" exit 1 fi }📝 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.copy_dir() { if [ ! -d "$1" ]; then echo "Error: Source directory $1 does not exist" exit 1 fi if [ -e "$2" ]; then echo "Warning: Destination $2 already exists, removing..." rm -rf "$2" fi if ! cp -r "$1" "$2"; then echo "Failed to copy from $1 to $2" exit 1 fi }
3-9: 🛠️ Refactor suggestion
Add directory validation checks
The script assumes it's being run from the correct directory. Add validation to ensure required directories exist before proceeding.
# Set common paths STANDALONE_DIR="$PWD/.next/standalone" APP_DIR="$STANDALONE_DIR/apps/cache-testing-15-app" PUBLIC_DIR="$PWD/public" STATIC_DIR="$PWD/.next/static" FETCH_CACHE_DIR="$PWD/.next/cache/fetch-cache" INSTANCES_DIR="$PWD/.next/__instances" + +# Validate required directories exist +for dir in "$PUBLIC_DIR" "$STATIC_DIR" "$FETCH_CACHE_DIR"; do + if [ ! -d "$dir" ]; then + echo "Error: Required directory $dir does not exist" + exit 1 + fi +done📝 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.# Set common paths STANDALONE_DIR="$PWD/.next/standalone" APP_DIR="$STANDALONE_DIR/apps/cache-testing-15-app" PUBLIC_DIR="$PWD/public" STATIC_DIR="$PWD/.next/static" FETCH_CACHE_DIR="$PWD/.next/cache/fetch-cache" INSTANCES_DIR="$PWD/.next/__instances" # Validate required directories exist for dir in "$PUBLIC_DIR" "$STATIC_DIR" "$FETCH_CACHE_DIR"; do if [ ! -d "$dir" ]; then echo "Error: Required directory $dir does not exist" exit 1 fi done
1-2: 🛠️ Refactor suggestion
Add proper script structure and error handling
The script would benefit from better error handling, cleanup, and usage information.
#!/bin/bash +set -euo pipefail + +# Cleanup function +cleanup() { + local exit_code=$? + echo "Cleaning up..." + # Add cleanup logic here if needed + exit $exit_code +} + +# Help function +show_help() { + echo "Usage: $(basename "$0")" + echo "Creates Next.js standalone application instances for testing" + echo + echo "The script will create instances on ports 3000 and 3001" +} + +# Set up trap handlers +trap cleanup EXIT +trap 'exit 1' INT TERM + +# Parse command line arguments +while getopts "h" opt; do + case $opt in + h) + show_help + exit 0 + ;; + *) + show_help + exit 1 + ;; + esac +done📝 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.#!/bin/bash set -euo pipefail # Cleanup function cleanup() { local exit_code=$? echo "Cleaning up..." # Add cleanup logic here if needed exit $exit_code } # Help function show_help() { echo "Usage: $(basename "$0")" echo "Creates Next.js standalone application instances for testing" echo echo "The script will create instances on ports 3000 and 3001" } # Set up trap handlers trap cleanup EXIT trap 'exit 1' INT TERM # Parse command line arguments while getopts "h" opt; do case $opt in h) show_help exit 0 ;; *) show_help exit 1 ;; esac doneapps/cache-testing-15-app/cache-handler-redis-strings.mjs (5)
14-18: 🛠️ Refactor suggestion
Consider improving type safety and validation
While the Redis client configuration is good, we can improve type safety and add URL validation.
Consider these improvements:
- /** @type {import("redis").RedisClientType} */ + /** @type {import("redis").RedisClientType<{ [key: string]: unknown }>} */ const client = createClient({ url: process.env.REDIS_URL, name: `cache-handler:${PREFIX}${process.env.PORT ?? process.pid}`, + socket: { + reconnectStrategy: (retries) => Math.min(retries * 50, 1000) + } }); + try { + const url = new URL(process.env.REDIS_URL); + if (!['redis:', 'rediss:'].includes(url.protocol)) { + throw new Error('Invalid Redis URL protocol'); + } + } catch (e) { + throw new Error(`Invalid REDIS_URL: ${e.message}`); + }Committable suggestion was skipped due to low confidence.
7-10:
⚠️ Potential issueStrengthen the REDIS_URL validation
The current warning for missing REDIS_URL is insufficient as the application cannot function without it. Consider throwing an error or providing more detailed guidance.
Consider this implementation:
CacheHandler.onCreation(async () => { if (!process.env.REDIS_URL) { - console.warn('Make sure that REDIS_URL is added to the .env.local file and loaded properly.'); + throw new Error( + 'REDIS_URL environment variable is required but not set. ' + + 'Please add it to your .env.local file and ensure it points to a valid Redis instance.' + ); }📝 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.CacheHandler.onCreation(async () => { if (!process.env.REDIS_URL) { throw new Error( 'REDIS_URL environment variable is required but not set. ' + 'Please add it to your .env.local file and ensure it points to a valid Redis instance.' ); }
20-25:
⚠️ Potential issueEnhance error handling strategy
The current error handling might miss critical errors when debug mode is off. Consider implementing a more robust error handling strategy.
Consider this implementation:
- client.on('error', (error) => { - if (typeof process.env.NEXT_PRIVATE_DEBUG_CACHE !== 'undefined') { - console.error('Redis client error:', error); - } - }); + client.on('error', (error) => { + // Always log critical errors + console.error('[Redis Cache] Critical error:', error.message); + + // Detailed logging in debug mode + if (typeof process.env.NEXT_PRIVATE_DEBUG_CACHE !== 'undefined') { + console.error('[Redis Cache] Error details:', error); + } + + // Emit event for external error tracking + process.emit('redis-cache-error', error); + });📝 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.// Redis won't work without error handling. https://github.com/redis/node-redis?tab=readme-ov-file#events client.on('error', (error) => { // Always log critical errors console.error('[Redis Cache] Critical error:', error.message); // Detailed logging in debug mode if (typeof process.env.NEXT_PRIVATE_DEBUG_CACHE !== 'undefined') { console.error('[Redis Cache] Error details:', error); } // Emit event for external error tracking process.emit('redis-cache-error', error); });
27-29: 🛠️ Refactor suggestion
Add connection timeout and retry mechanism
The connection handling could benefit from a timeout and retry mechanism to handle connection failures gracefully.
Consider implementing connection timeout and retry logic:
- console.info('Connecting Redis client...'); - await client.connect(); - console.info('Redis client connected.'); + console.info('[Redis Cache] Connecting to Redis...'); + try { + await Promise.race([ + client.connect(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Redis connection timeout')), 5000) + ) + ]); + console.info('[Redis Cache] Connected successfully'); + } catch (error) { + console.error('[Redis Cache] Connection failed:', error.message); + throw error; + }Committable suggestion was skipped due to low confidence.
31-39: 🧹 Nitpick (assertive)
Document TTL configuration
The TTL configuration would benefit from documentation explaining the rationale behind the values.
Add JSDoc comments to explain the TTL configuration:
+ /** + * Creates a Redis cache handler with the following configuration: + * - keyPrefix: Namespace for Redis keys to avoid conflicts + * - TTL: Default stale age of 60 seconds + * - Expiration: Double the stale age to allow for background revalidation + */ return { handlers: [redisHandler], ttl: { defaultStaleAge: 60, estimateExpireAge: (staleAge) => staleAge * 2 }, };📝 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 redisHandler = createRedisHandler({ client, keyPrefix: PREFIX, }); /** * Creates a Redis cache handler with the following configuration: * - keyPrefix: Namespace for Redis keys to avoid conflicts * - TTL: Default stale age of 60 seconds * - Expiration: Double the stale age to allow for background revalidation */ return { handlers: [redisHandler], ttl: { defaultStaleAge: 60, estimateExpireAge: (staleAge) => staleAge * 2 }, };apps/cache-testing-15-app/src/components/cache-state-watcher.tsx (3)
44-50: 🧹 Nitpick (assertive)
Enhance accessibility and semantic structure.
While the component structure is clean, it could benefit from improved accessibility.
return ( - <div> + <div role="status" aria-live="polite"> <div data-pw="cache-state">Cache state: {cacheState}</div> <div data-pw="stale-after">Stale after: {countDown}</div> </div> );This change makes the component more accessible by announcing state changes to screen readers.
📝 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.return ( <div role="status" aria-live="polite"> <div data-pw="cache-state">Cache state: {cacheState}</div> <div data-pw="stale-after">Stale after: {countDown}</div> </div> ); }
7-9: 🛠️ Refactor suggestion
Initialize state with meaningful default values.
The empty string initialization for both states could cause unnecessary re-renders and UI flicker. Consider initializing with the actual initial values that match the first render.
- const [cacheState, setCacheState] = useState(''); - const [countDown, setCountDown] = useState(''); + const [cacheState, setCacheState] = useState('fresh'); + const [countDown, setCountDown] = useState('0.000');Committable suggestion was skipped due to low confidence.
11-42:
⚠️ Potential issueAdd input validation and optimize performance.
Several improvements could enhance the robustness and performance of this component:
- Add input validation for the
timeprop:useEffect(() => { + if (!Number.isFinite(time) || time < 0) { + console.error('Invalid time prop provided'); + return; + } if (!Number.isFinite(revalidateAfter)) {
- Type the animation frame ID properly:
- let id = -1; + let id: number = -1;
- Optimize performance for large revalidation intervals:
function check(): void { const now = Date.now(); + const timeRemaining = time + revalidateAfter - now; - setCountDown(Math.max(0, (time + revalidateAfter - now) / 1000).toFixed(3)); + setCountDown(Math.max(0, timeRemaining / 1000).toFixed(3)); - if (now > time + revalidateAfter) { + if (timeRemaining <= 0) { setCacheState('stale'); return; } setCacheState('fresh'); - id = requestAnimationFrame(check); + // Reduce update frequency when far from revalidation + if (timeRemaining > 1000) { + id = window.setTimeout(check, 1000); + } else { + id = requestAnimationFrame(check); + } }Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/src/components/revalidate-button.tsx (3)
1-14: 🧹 Nitpick (assertive)
LGTM! Well-structured type definitions with proper constraints.
The type definitions effectively model the different revalidation capabilities between App and Pages router. The discriminated union pattern with
nextApiensures type safety.The explicit JSX import from 'react' is unnecessary in TSX files as it's globally available. Consider removing it:
-import { type JSX, useState } from 'react'; +import { useState } from 'react';📝 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.'use client'; import { usePathname } from 'next/navigation'; import { useState } from 'react'; type RevalidateButtonAppProps = { nextApi: 'app'; type: 'path' | 'tag'; }; type RevalidateButtonPagesProps = { nextApi: 'pages'; type: 'path'; };
44-52: 🧹 Nitpick (assertive)
Enhance accessibility with ARIA attributes.
While the component structure is clean, it could benefit from improved accessibility.
Consider these accessibility enhancements:
return ( <div> - <button data-pw={`revalidate-button-${type}`} onClick={handleRevalidation} type="button"> + <button + data-pw={`revalidate-button-${type}`} + onClick={handleRevalidation} + type="button" + aria-label={`Revalidate cache by ${type}`} + > Revalidate {type} </button> - <div data-pw={`is-revalidated-by-${type}`}>{revalidation}</div> + <div + data-pw={`is-revalidated-by-${type}`} + role="status" + aria-live="polite" + > + {revalidation} + </div> </div> );📝 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.return ( <div> <button data-pw={`revalidate-button-${type}`} onClick={handleRevalidation} type="button" aria-label={`Revalidate cache by ${type}`} > Revalidate {type} </button> <div data-pw={`is-revalidated-by-${type}`} role="status" aria-live="polite" > {revalidation} </div> </div> ); }
24-42:
⚠️ Potential issueConsider improving error handling and user experience.
The revalidation handler has several areas for improvement:
- Network errors are not caught
- Users have no feedback during revalidation
- API response type could be more strictly typed
- Multiple rapid clicks could lead to race conditions
Consider this improved implementation:
+interface RevalidateResponse { + now: string; +} + function handleRevalidation(): void { + setRevalidation('Revalidating...'); + const searchParams = new URLSearchParams(); if (pathname) { searchParams.set(type, pathname); } - fetch(`/api/revalidate-${nextApi}?${searchParams.toString()}`).then(async (result) => { + fetch(`/api/revalidate-${nextApi}?${searchParams.toString()}`) + .then(async (result) => { if (!result.ok) { - setRevalidation('Fail to revalidate'); - return; + throw new Error(`Failed to revalidate: ${result.statusText}`); } - const json = (await result.json()) as { now: string }; + const json = await result.json() as RevalidateResponse; setRevalidation(`Revalidated at ${json.now}`); + }) + .catch((error) => { + console.error('Revalidation error:', error); + setRevalidation(`Failed to revalidate: ${error.message}`); }); }📝 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.interface RevalidateResponse { now: string; } function handleRevalidation(): void { setRevalidation('Revalidating...'); const searchParams = new URLSearchParams(); if (pathname) { searchParams.set(type, pathname); } fetch(`/api/revalidate-${nextApi}?${searchParams.toString()}`) .then(async (result) => { if (!result.ok) { throw new Error(`Failed to revalidate: ${result.statusText}`); } const json = await result.json() as RevalidateResponse; setRevalidation(`Revalidated at ${json.now}`); }) .catch((error) => { console.error('Revalidation error:', error); setRevalidation(`Failed to revalidate: ${error.message}`); }); }apps/cache-testing-15-app/src/app/app/randomHex/[length]/page.tsx (5)
10-14:
⚠️ Potential issueSimplify types and static params generation by removing unnecessary Promises.
The current implementation unnecessarily wraps the params in Promises, which adds complexity without benefits.
Consider this simplified implementation:
-type PageParams = { params: Promise<{ length: string }> }; +type PageParams = { params: { length: string } }; export function generateStaticParams(): PageParams['params'][] { - return lengthSteps.map((length) => Promise.resolve({ length: `${length}` })); + return lengthSteps.map((length) => ({ length: `${length}` })); }📝 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.type PageParams = { params: { length: string } }; export function generateStaticParams(): PageParams['params'][] { return lengthSteps.map((length) => ({ length: `${length}` })); }
8-8: 🛠️ Refactor suggestion
Consider improving readability and type safety of lengthSteps.
While the current implementation works, it could be more readable and type-safe.
Consider this alternative implementation:
-const lengthSteps = new Array(5).fill(0).map((_, i) => 10 ** (i + 1)); +const lengthSteps: readonly number[] = [10, 100, 1000, 10000, 100000];Or if dynamic generation is preferred:
-const lengthSteps = new Array(5).fill(0).map((_, i) => 10 ** (i + 1)); +const lengthSteps: readonly number[] = Array.from( + { length: 5 }, + (_, i) => Math.pow(10, i + 1) +);📝 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 lengthSteps: readonly number[] = [10, 100, 1000, 10000, 100000];
19-19:
⚠️ Potential issueAdd type validation for the length parameter.
The length parameter from the URL should be validated to ensure it's a valid number.
Add validation before using the length parameter:
const { length } = params; +const numericLength = parseInt(length, 10); +if (isNaN(numericLength) || !lengthSteps.includes(numericLength)) { + notFound(); +}Committable suggestion was skipped due to low confidence.
41-43: 🛠️ Refactor suggestion
Consider adding error boundary and loading state.
The Suspense component should have proper error handling and loading state for better user experience.
Consider wrapping the Suspense with an error boundary and adding a loading indicator:
+import { ErrorBoundary } from 'cache-testing-15-app/components/error-boundary'; +import { LoadingSpinner } from 'cache-testing-15-app/components/loading-spinner'; -<Suspense fallback={null}> +<ErrorBoundary fallback={<div>Error loading cache state</div>}> + <Suspense fallback={<LoadingSpinner />}> <CacheStateWatcher revalidateAfter={Number.POSITIVE_INFINITY} time={props.unixTimeMs} /> - </Suspense> + </Suspense> +</ErrorBoundary>Committable suggestion was skipped due to low confidence.
23-23:
⚠️ Potential issueReplace hardcoded URL with environment variable.
Using a hardcoded localhost URL could cause issues in production environments.
Consider using an environment variable:
-const url = new URL(path, 'http://localhost:8081'); +const url = new URL(path, process.env.NEXT_PUBLIC_API_BASE_URL);Committable suggestion was skipped due to low confidence.
apps/cache-testing-15-app/tests/test-helpers.ts (2)
3-7: 🛠️ Refactor suggestion
Consider adding timeout and error handling.
While the implementation is solid, consider these improvements for robustness:
- Add timeout to the expect assertion
- Add error handling for failed interactions
export async function revalidateByTag(page: Page) { + try { await page.getByTestId('revalidate-button-tag').click(); - await expect(page.getByTestId('is-revalidated-by-tag')).toContainText('Revalidated at'); + await expect(page.getByTestId('is-revalidated-by-tag')).toContainText('Revalidated at', { + timeout: 5000 + }); + } catch (error) { + throw new Error(`Failed to revalidate by tag: ${error.message}`); + } }📝 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.export async function revalidateByTag(page: Page) { try { await page.getByTestId('revalidate-button-tag').click(); await expect(page.getByTestId('is-revalidated-by-tag')).toContainText('Revalidated at', { timeout: 5000 }); } catch (error) { throw new Error(`Failed to revalidate by tag: ${error.message}`); } }
9-13: 🛠️ Refactor suggestion
Consider reducing code duplication with a shared implementation.
The function is nearly identical to
revalidateByTag. Consider creating a shared implementation:+async function revalidate(page: Page, by: 'tag' | 'path') { + try { + await page.getByTestId(`revalidate-button-${by}`).click(); + await expect(page.getByTestId(`is-revalidated-by-${by}`)).toContainText('Revalidated at', { + timeout: 5000 + }); + } catch (error) { + throw new Error(`Failed to revalidate by ${by}: ${error.message}`); + } +} + -export async function revalidateByPath(page: Page) { - await page.getByTestId('revalidate-button-path').click(); - - await expect(page.getByTestId('is-revalidated-by-path')).toContainText('Revalidated at'); -} +export const revalidateByPath = (page: Page) => revalidate(page, 'path'); +export const revalidateByTag = (page: Page) => revalidate(page, 'tag');Committable suggestion was skipped due to low confidence.
internal/next-lru-cache/src/cache-types/next-cache-handler-value.ts (5)
25-29: 🧹 Nitpick (assertive)
Add html null check for consistency
While this implementation handles pageData well, it should maintain consistency with html null checking.
Consider this minor improvement:
case 'PAGE': { const pageDataLength = value.pageData ? JSON.stringify(value.pageData).length : 0; - return value.html.length + pageDataLength; + const htmlLength = value.html?.length ?? 0; + return htmlLength + pageDataLength; }📝 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.case 'PAGE': { const pageDataLength = value.pageData ? JSON.stringify(value.pageData).length : 0; const htmlLength = value.html?.length ?? 0; return htmlLength + pageDataLength; }
18-20:
⚠️ Potential issueAdd null checks and proper type validation
The current implementation might throw errors if
value.htmlis undefined, and the handling ofrscDatacould be more robust.Consider this safer implementation:
case 'APP_PAGE': { - return value.html.length + (JSON.stringify(value.rscData)?.length || 0); + const htmlSize = value.html?.length ?? 0; + const rscSize = value.rscData ? JSON.stringify(value.rscData).length : 0; + return htmlSize + rscSize; }📝 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.case 'APP_PAGE': { const htmlSize = value.html?.length ?? 0; const rscSize = value.rscData ? JSON.stringify(value.rscData).length : 0; return htmlSize + rscSize; }
31-35:
⚠️ Potential issueAdd null check for body property
Maintain consistency with null checking across all cases.
Apply this improvement:
case 'ROUTE': case 'APP_ROUTE': { - return value.body.length; + return value.body?.length ?? 0; }📝 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.case 'ROUTE': case 'APP_ROUTE': { // Size based on the length of the body return value.body?.length ?? 0; }
45-46:
⚠️ Potential issueRemove @ts-expect-error using proper exhaustive type checking
The PR objectives mention removing all @ts-expect-error instances. This can be achieved using proper TypeScript patterns.
Replace with exhaustive type checking:
- // @ts-expect-error - throw new Error(`Invalid kind: ${value.kind}`); + const exhaustiveCheck: never = value.kind; + throw new Error(`Invalid kind: ${exhaustiveCheck}`);📝 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 exhaustiveCheck: never = value.kind; throw new Error(`Invalid kind: ${exhaustiveCheck}`);
21-23:
⚠️ Potential issueMaintain consistency with null checks
Similar to the APP_PAGE case, this implementation needs more robust null checking.
Apply similar improvements:
case 'PAGES': { - return value.html.length + (JSON.stringify(value.pageData)?.length || 0); + const htmlSize = value.html?.length ?? 0; + const pageDataSize = value.pageData ? JSON.stringify(value.pageData).length : 0; + return htmlSize + pageDataSize; }📝 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.case 'PAGES': { const htmlSize = value.html?.length ?? 0; const pageDataSize = value.pageData ? JSON.stringify(value.pageData).length : 0; return htmlSize + pageDataSize; }.github/workflows/tests.yml (1)
83-83: 💡 Codebase verification
Both workspaces need e2e test coverage in CI
The verification revealed that both
apps/cache-testingandapps/cache-testing-15-apphave identical e2e test configurations, but onlyapps/cache-testingis being tested in CI. Since these are separate test suites for different Next.js versions, both should be executed in the workflow.
- Add a new step to run e2e tests for
apps/cache-testing-15-appworkspace:- name: Run e2e tests (Next.js 15) run: | npm run e2e -w apps/cache-testing-15-app🔗 Analysis chain
LGTM! Verify if other workspaces need e2e tests.
The workspace-specific e2e test command is correct and aligns with the PR's objective of adding Next.js 15 support.
Let's check if there are other workspaces with e2e tests that might need to be included:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other potential e2e test configurations in workspaces # Expected: Find any package.json files containing e2e-related scripts # Search for package.json files containing e2e scripts echo "Searching for e2e scripts in workspaces:" fd package.json apps/ -x jq -r 'select(.scripts | objects | to_entries | map(select(.key | contains("e2e"))) | length > 0) | input_filename'Length of output: 304
Script:
#!/bin/bash # Let's examine the e2e scripts in both package.json files to understand their purpose echo "Content of apps/cache-testing/package.json scripts:" jq -r '.scripts | to_entries | map(select(.key | contains("e2e"))) | .[] | "\(.key): \(.value)"' apps/cache-testing/package.json echo -e "\nContent of apps/cache-testing-15-app/package.json scripts:" jq -r '.scripts | to_entries | map(select(.key | contains("e2e"))) | .[] | "\(.key): \(.value)"' apps/cache-testing-15-app/package.json # Let's also check if these are referenced in the workflow file echo -e "\nChecking workflow file for e2e test references:" grep -A 2 "e2e" .github/workflows/tests.ymlLength of output: 1083
apps/cache-testing-15-app/src/globals.css (6)
63-68:
⚠️ Potential issueUse the defined --font-mono CSS variable for consistency.
The font family is hardcoded while a comprehensive monospace font stack is already defined in the CSS variables.
html, body { max-width: 100vw; overflow-x: hidden; - font-family: "Courier New", Courier, monospace; + font-family: var(--font-mono); }
4-6: 🛠️ Refactor suggestion
Optimize monospace font stack for better cross-platform support.
Consider enhancing the monospace font stack by adding system fonts and optimizing the order:
--font-mono: - ui-monospace, Menlo, Monaco, "Cascadia Mono", "Segoe UI Mono", "Roboto Mono", "Oxygen Mono", - "Ubuntu Monospace", "Source Code Pro", "Fira Mono", "Droid Sans Mono", "Courier New", monospace; + ui-monospace, "SF Mono", SFMono-Regular, Consolas, Menlo, Monaco, "Cascadia Mono", "Segoe UI Mono", + "Roboto Mono", "Oxygen Mono", "Ubuntu Monospace", "Source Code Pro", "Fira Mono", "Droid Sans Mono", + "Courier New", monospace;📝 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.--font-mono: ui-monospace, "SF Mono", SFMono-Regular, Consolas, Menlo, Monaco, "Cascadia Mono", "Segoe UI Mono", "Roboto Mono", "Oxygen Mono", "Ubuntu Monospace", "Source Code Pro", "Fira Mono", "Droid Sans Mono", "Courier New", monospace;
57-61: 🛠️ Refactor suggestion
Consider enhancing the CSS reset for better cross-browser consistency.
The current reset is good but could be more comprehensive for better cross-browser and cross-device compatibility.
* { box-sizing: border-box; padding: 0; margin: 0; + line-height: 1.5; + -webkit-text-size-adjust: 100%; + word-break: break-word; }📝 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.* { box-sizing: border-box; padding: 0; margin: 0; line-height: 1.5; -webkit-text-size-adjust: 100%; word-break: break-word; }
70-74: 🛠️ Refactor suggestion
Consider using responsive margins for better mobile support.
The fixed margin of 16px might not be optimal for all screen sizes.
body { color: rgb(var(--foreground-rgb)); background: linear-gradient(to bottom, transparent, rgb(var(--background-end-rgb))) rgb(var(--background-start-rgb)); - margin: 16px; + margin: clamp(8px, 2vw, 16px); }📝 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.body { color: rgb(var(--foreground-rgb)); background: linear-gradient(to bottom, transparent, rgb(var(--background-end-rgb))) rgb(var(--background-start-rgb)); margin: clamp(8px, 2vw, 16px); }
87-89: 🛠️ Refactor suggestion
Configure grid layout properties for better structure.
The grid display lacks specific configuration which might lead to inconsistent layouts.
.header { display: grid; + grid-template-columns: repeat(auto-fit, minmax(250px, 1fr)); + gap: 1rem; + align-items: center; }📝 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..header { display: grid; grid-template-columns: repeat(auto-fit, minmax(250px, 1fr)); gap: 1rem; align-items: center; }
76-79:
⚠️ Potential issueAdd focus and hover states for better accessibility.
Interactive states are missing for anchor tags, which are important for accessibility and user experience.
a { color: inherit; text-decoration: none; + transition: opacity 0.2s ease; +} + +a:hover { + opacity: 0.8; +} + +a:focus-visible { + outline: 2px solid currentColor; + outline-offset: 2px; }📝 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.a { color: inherit; text-decoration: none; transition: opacity 0.2s ease; } a:hover { opacity: 0.8; } a:focus-visible { outline: 2px solid currentColor; outline-offset: 2px; }packages/cache-handler/src/functions/nesh-classic-cache.ts (1)
276-279:
⚠️ Potential issueRemove @ts-expect-error by properly typing the cache options.
The PR objectives mention removing all @ts-expect-error instances. Consider updating the type definitions to properly include these Next.js 15 specific cache parameters.
Consider creating an interface for the cache options:
interface CacheGetOptions { revalidate: number | false; tags: string[]; kindHint: 'fetch'; fetchUrl: string; }apps/cache-testing-15-app/run-app-instances.ts (3)
58-60: 🧹 Nitpick (assertive)
Remove unnecessary 'await' when sending the response
The
reply.sendmethod in Fastify does not return a Promise, so usingawaitis unnecessary. Removingawaitsimplifies the code without affecting functionality.Apply this diff to remove the unnecessary
await:app.get('/', async (_request, reply) => { - await reply.send('ok'); + reply.send('ok'); });📝 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.app.get('/', async (_request, reply) => { reply.send('ok'); });
75-77: 🧹 Nitpick (assertive)
Remove unnecessary 'await' before 'reply.send'
Similar to the previous comment, the
reply.sendmethod does not return a Promise. Removingawaithere simplifies the code.Apply this diff:
// Workaround for unstable tests await scheduler.wait(1000); - await reply.code(200).send({ restarted: name }); + reply.code(200).send({ restarted: name });Committable suggestion was skipped due to low confidence.
62-81: 🛠️ Refactor suggestion
Refactor '/restart/:port' route to use async/await for better readability
The current implementation uses callback-based functions, which can lead to complex nested logic and potential errors. By promisifying
pm2.restart, you can leverageasync/awaitfor cleaner code and improved error handling.Apply this diff to refactor the code:
+import { promisify } from 'node:util'; + +const pm2Restart = promisify(pm2.restart); app.get('/restart/:port', async (request, reply) => { const { port } = request.params as { port: string }; const name = getNameByPort(port); - pm2.restart(name, (restartError: unknown) => { - if (restartError) { - console.error(restartError); - reply.code(500).send({ status: 'error' }); - return; - } - - // workaround for unstable tests - scheduler.wait(1000).then(async () => { - await reply.code(200).send({ restarted: name }); - }); - }); - - return reply; + try { + await pm2Restart(name); + } catch (restartError) { + console.error(restartError); + return reply.code(500).send({ status: 'error' }); + } + + // Workaround for unstable tests + await scheduler.wait(1000); + return reply.code(200).send({ restarted: name }); });📝 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.import { promisify } from 'node:util'; const pm2Restart = promisify(pm2.restart); app.get('/restart/:port', async (request, reply) => { const { port } = request.params as { port: string }; const name = getNameByPort(port); try { await pm2Restart(name); } catch (restartError) { console.error(restartError); return reply.code(500).send({ status: 'error' }); } // Workaround for unstable tests await scheduler.wait(1000); return reply.code(200).send({ restarted: name }); });apps/cache-testing-15-app/src/utils/create-get-data.ts (3)
86-88:
⚠️ Potential issueAssess the necessity of including 'path' in the returned data for security reasons.
Including the
pathvariable in the response may expose internal routing details to the client. Review if this is necessary, and consider omitting it to enhance security if it's not required.
23-27: 🧹 Nitpick (assertive)
Rename 'splittedPath' to 'splitPath' for correct grammar and readability.
The word 'splitted' is not commonly used; 'split' serves as both the present and past tense. Renaming improves clarity and aligns with standard naming conventions.
Apply this diff to rename the variable:
- const splittedPath = path.split('/'); + const splitPath = path.split('/'); - splittedPath[0] = `next-15-${splittedPath[0]}`; + splitPath[0] = `next-15-${splitPath[0]}`; - const pathAndTag = `/${splittedPath.join('/')}/${normalizeSlug(slug)}`; + const pathAndTag = `/${splitPath.join('/')}/${normalizeSlug(slug)}`;📝 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 splitPath = path.split('/'); splitPath[0] = `next-15-${splitPath[0]}`; const pathAndTag = `/${splitPath.join('/')}/${normalizeSlug(slug)}`;
37-40: 💡 Codebase verification
Consider migrating from
unstable_cachetoneshCacheThe codebase already includes and uses
neshCachefrom@neshca/cache-handler/functionsas a more stable alternative tounstable_cache. The documentation found in the repository specifically recommends usingneshCacheto "replaceunstable_cachefor more control over caching." The code shows both implementations side by side, withneshCachebeing the preferred method.
apps/cache-testing-15-app/src/utils/create-get-data.ts: Already hasneshCacheimplementationapps/cache-testing/src/utils/create-get-data.ts: Already hasneshCacheimplementationWhile the current usage of
unstable_cacheis technically correct (passing tags both as second argument and in options is valid), it's recommended to switch completely to the more stableneshCacheimplementation that's already present in the codebase.🔗 Analysis chain
Verify the usage of 'unstable_cache' to ensure correct caching behavior.
Ensure that passing
tagsboth as the second argument and within the options object aligns with theunstable_cacheAPI specifications. Incorrect usage may lead to unexpected caching behaviors.Run the following script to check all usages of
unstable_cachein the codebase:This script helps verify if the
unstable_cachefunction is consistently and correctly used throughout the codebase.
Let me gather more information about the
unstable_cacheAPI specification by searching for its imports and type definitions.
Let me check the Next.js documentation by examining any README files or documentation in the repository that might provide guidance about the
unstable_cacheusage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of 'unstable_cache' to verify parameter usage. # Search for 'unstable_cache' and display surrounding lines for context. rg 'unstable_cache\(' -A 5 -B 2Length of output: 1384
Script:
#!/bin/bash # Search for unstable_cache imports and type definitions rg "import.*unstable_cache" -A 2 # Search for any documentation or comments about unstable_cache rg "unstable_cache" --type ts --type tsx -B 3 -A 3Length of output: 757
Script:
#!/bin/bash # Search for any documentation files fd -e md -x cat {} \; | rg -i "unstable.?cache" -B 3 -A 3 # Let's also check the full context of the create-get-data files cat apps/cache-testing-15-app/src/utils/create-get-data.ts cat apps/cache-testing/src/utils/create-get-data.tsLength of output: 6732
apps/cache-testing-15-app/cache-handler-redis-stack.mjs (3)
22-22:
⚠️ Potential issueCorrect the URL typo in the comment
There's a typographical error in the URL within the comment. The correct URL should accurately reference the Redis client events documentation.
Apply this diff to fix the typo:
-// Redis won't work without error handling. https://github.com/redis/node-redis?tab=readme-ov-file#events +// Redis won't work without error handling. https://github.com/redis/node-redis#events📝 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.// Redis won't work without error handling. https://github.com/redis/node-redis#events
36-39: 🛠️ Refactor suggestion
Implement a timeout for Redis client connection
Waiting indefinitely for the Redis client to connect can block the server startup if the Redis server is unresponsive. Implementing a timeout prevents potential hangs and improves the robustness of your application.
Apply this diff to add a timeout to the Redis client connection:
-// Caveat: This will block the server from starting until the client is connected. -// And there is no timeout. Make your own timeout if needed. -await client.connect(); +// Implement a timeout for the Redis client connection. +const connectPromise = client.connect(); +const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error('Redis client connection timed out')), 5000) +); +await Promise.race([connectPromise, timeoutPromise]);📝 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.// Wait for the client to connect. // Implement a timeout for the Redis client connection. const connectPromise = client.connect(); const timeoutPromise = new Promise((_, reject) => setTimeout(() => reject(new Error('Redis client connection timed out')), 5000) ); await Promise.race([connectPromise, timeoutPromise]);
46-54: 🛠️ Refactor suggestion
Use 'await' syntax for disconnecting the Redis client
Since the function is asynchronous, using the
awaitkeyword withclient.disconnect()enhances readability and consistency in your asynchronous code.Apply this diff to refactor the disconnection logic:
-client - .disconnect() - .then(() => { - console.info('Redis client disconnected.'); - }) - .catch(() => { - console.warn('Failed to quit the Redis client after failing to connect.'); - }); +try { + await client.disconnect(); + console.info('Redis client disconnected.'); +} catch { + console.warn('Failed to disconnect the Redis client after failing to connect.'); +}📝 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.try { await client.disconnect(); console.info('Redis client disconnected.'); } catch { console.warn('Failed to disconnect the Redis client after failing to connect.'); } }apps/cache-testing-15-app/cache-handler-redis-stack.js (4)
37-39: 🛠️ Refactor suggestion
Implement a timeout for Redis client connection to prevent blocking
Connecting to the Redis client without a timeout can indefinitely block the server startup if the Redis server is unreachable. To prevent this, consider implementing a timeout for the
connectmethod.Here's how you might implement a timeout using
Promise.race:// Wait for the client to connect. -// Caveat: This will block the server from starting until the client is connected. -// And there is no timeout. Make your own timeout if needed. -await client.connect(); +const connectPromise = client.connect(); +const timeout = 5000; // Timeout in milliseconds +const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error('Redis connection timed out')), timeout) +); +await Promise.race([connectPromise, timeoutPromise]); console.info('Redis client connected.');📝 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 connectPromise = client.connect(); const timeout = 5000; // Timeout in milliseconds const timeoutPromise = new Promise((_, reject) => setTimeout(() => reject(new Error('Redis connection timed out')), timeout) ); await Promise.race([connectPromise, timeoutPromise]);
74-77: 🧹 Nitpick (assertive)
Consider making TTL configuration more flexible
The TTL settings are hardcoded, which may not accommodate different caching requirements. Consider allowing dynamic configuration through environment variables or a configuration file for greater flexibility.
Example:
return { handlers: [handler], - ttl: { defaultStaleAge: 60, estimateExpireAge: (staleAge) => staleAge * 2 }, + ttl: { + defaultStaleAge: process.env.DEFAULT_STALE_AGE ? parseInt(process.env.DEFAULT_STALE_AGE) : 60, + estimateExpireAge: (staleAge) => staleAge * 2, + }, };📝 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.return { handlers: [handler], ttl: { defaultStaleAge: process.env.DEFAULT_STALE_AGE ? parseInt(process.env.DEFAULT_STALE_AGE) : 60, estimateExpireAge: (staleAge) => staleAge * 2, }, };
17-20:
⚠️ Potential issueFix incorrect use of 'name' option in Redis client configuration
The
nameoption is not a valid parameter forcreateClientin the Redis client library. To set the client name, you should use theclientSetNamemethod after the client has connected.Apply this diff to fix the issue:
client = createClient({ url: process.env.REDIS_URL, - name: `cache-handler:${process.env.PORT ?? process.pid}`, }); try { console.info('Connecting Redis client...'); // Wait for the client to connect. await client.connect(); + await client.clientSetName(`cache-handler:${process.env.PORT ?? process.pid}`); console.info('Redis client connected.'); } catch (error) { console.warn('Failed to connect Redis client:', error);Committable suggestion was skipped due to low confidence.
23-27:
⚠️ Potential issueEnsure critical errors are logged regardless of environment variable
Currently, Redis client errors are logged only if
process.env.NEXT_PRIVATE_DEBUG_CACHEis defined. This might cause important errors to be missed in environments where this variable is not set. Consider logging errors by default or adjusting the condition to ensure critical errors are not suppressed.Apply this diff to log errors by default:
client.on('error', (error) => { - if (typeof process.env.NEXT_PRIVATE_DEBUG_CACHE !== 'undefined') { + if (!process.env.NEXT_PRIVATE_DEBUG_CACHE) { console.error('Redis client error:', error); } });Alternatively, log errors unconditionally:
client.on('error', (error) => { console.error('Redis client error:', error); });Committable suggestion was skipped due to low confidence.
internal/next-common/src/next-common.ts (1)
90-95:
⚠️ Potential issueInconsistent Units in 'lastModified' Timestamp
The
lastModifiedproperty inCacheHandlerValueis documented as a timestamp in milliseconds, whereas thelastModifiedAtproperty inLifespanParametersis defined in seconds. This inconsistency in time units could lead to confusion or bugs when performing time calculations.Please standardize the time units for these properties to ensure consistency across the codebase.
apps/cache-testing-15-app/tests/app.spec.ts (5)
8-8: 🛠️ Refactor suggestion
Address the commented-out test path
The path
'/app/with-params/dynamic-false/200'is commented out because it fails with the native Next.js cache. If this is intended for future attention, consider adding a TODO comment or creating an issue to track this. This will help keep the codebase clean and ensure the issue is revisited.
36-36: 🧹 Nitpick (assertive)
Simplify assertions for clarity and idiomatic style
In several places, assertions are written by comparing a boolean expression to
true:expect(valueFromPageAfterReload - valueFromPage === 1).toBe(true);You can simplify these assertions for better readability and adherence to idiomatic testing practices:
-expect(valueFromPageAfterReload - valueFromPage === 1).toBe(true); +expect(valueFromPageAfterReload - valueFromPage).toBe(1);This change makes the intent clearer by directly asserting the expected value.
Also applies to: 62-62, 89-89, 116-116, 147-147, 178-178, 248-248, 274-274, 347-347, 375-375
139-140:
⚠️ Potential issueCorrect the usage of
Timers.scheduler.waitThe use of
Timers.scheduler.wait(1000);may not work as intended. To pause execution for 1 second, consider usingTimers.setTimeout:- await Timers.scheduler.wait(1000); + await Timers.setTimeout(1000);Alternatively, you can use a standard Promise-based timeout:
await new Promise((resolve) => setTimeout(resolve, 1000));Ensure that this delay effectively addresses the intermittent test failures, and consider investigating the root cause to find a more robust solution.
211-213: 🧹 Nitpick (assertive)
Replace console warning with a test assertion
Currently, the test includes a
console.warnstatement to indicate whenPage Bis more than one revalidation ahead ofPage A:if (valueFromPageB - valueFromPageA > 1) { console.warn('Page B is more than one revalidation ahead of page A.'); }Consider replacing this with an assertion to make the test fail under undesirable conditions, which is more effective for automated testing:
-if (valueFromPageB - valueFromPageA > 1) { - console.warn('Page B is more than one revalidation ahead of page A.'); -} +expect(valueFromPageB - valueFromPageA).toBeLessThanOrEqual(1);This change ensures that any unexpected behavior is caught during testing.
246-246:
⚠️ Potential issueCorrect variable reference from
appAtoappBIn this line,
valueFromPageBis obtained fromappAinstead ofappB:const valueFromPageB = Number.parseInt((await appA.getByTestId('data').innerText()).valueOf(), 10);This seems to be a typo. It should reference
appB:-const valueFromPageB = Number.parseInt((await appA.getByTestId('data').innerText()).valueOf(), 10); +const valueFromPageB = Number.parseInt((await appB.getByTestId('data').innerText()).valueOf(), 10);packages/cache-handler/src/cache-handler.ts (3)
422-422: 🧹 Nitpick (assertive)
Ensure
pageKindis validated before useBefore assigning
kind: pageKind, validate thatpageKindhas an expected value to avoid potential issues if the file content is invalid.Add validation for
pageKind:cacheHandlerValue = { lastModified: mtimeMs, lifespan: null, tags: [], value: { - kind: pageKind, + kind: pageKind === 'PAGE' || pageKind === 'PAGES' ? pageKind : 'PAGE', html: pageHtmlFile, pageData, postponed: undefined, headers: undefined, status: undefined, }, };Committable suggestion was skipped due to low confidence.
870-871:
⚠️ Potential issueInclude 'PAGES' kind in
switchstatement for consistencyThe kind
'PAGES'is checked elsewhere but not handled in theswitchstatement starting at line 836. This may lead to unhandled cases and potential bugs.Include
'PAGES'in theswitchstatement:switch (value?.kind) { case 'PAGE': + case 'PAGES': case 'APP_PAGE': { cacheHandlerValueTags = getTagsFromHeaders(value.headers ?? {}); break; } // Other cases... }Committable suggestion was skipped due to low confidence.
396-404:
⚠️ Potential issueHandle potential errors when reading
pageKindPathEnsure that the code properly handles the scenario where
pageKindPathdoes not exist or cannot be read. Currently, ifpageKindPathis missing, it may result in an unhandled exception.Consider adding error handling for reading
pageKindPath:const [pageHtmlFile, { mtimeMs }, pageData, pageKind] = await Promise.all([ pageHtmlHandle.readFile('utf-8'), pageHtmlHandle.stat(), fsPromises.readFile(pageDataPath, 'utf-8').then((data) => JSON.parse(data) as object), - fsPromises.readFile(pageKindPath, 'utf-8').then((data) => data.trim() as 'PAGES' | 'PAGE'), + fsPromises.readFile(pageKindPath, 'utf-8') + .then((data) => data.trim() as 'PAGES' | 'PAGE') + .catch(() => 'PAGE'), // Default to 'PAGE' if file is missing ]);📝 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 pageKindPath = path.join(CacheHandler.#serverDistDir, 'pages', `${cacheKey}.kind`); pageHtmlHandle = await fsPromises.open(pageHtmlPath, 'r'); const [pageHtmlFile, { mtimeMs }, pageData, pageKind] = await Promise.all([ pageHtmlHandle.readFile('utf-8'), pageHtmlHandle.stat(), fsPromises.readFile(pageDataPath, 'utf-8').then((data) => JSON.parse(data) as object), fsPromises.readFile(pageKindPath, 'utf-8') .then((data) => data.trim() as 'PAGES' | 'PAGE') .catch(() => 'PAGE'), // Default to 'PAGE' if file is missing
|
Can we include in this breaking change an export for |
I will do this in this PR |
|
Hey @better-salmon - could you tell me what is the status of this PR? Is it ready to use for testing in non-production environments? Or do you still expect significant changes other than tests and typing? |
|
Tested it quickly with cache-handler/redis-strings and next 15.0.3 stable Got the error 500 ⨯ TypeError: result.toUnchunkedString is not a function Happens only on the static pages node: v18.18.2 const payload = result.isDynamic ? null : result.toUnchunkedString();result typeof "string" here, it seems stupid fix fixes this problem, added it to cache-handler.mjs /** fix for cache-handler and next 15 bug */
String.prototype.toUnchunkedString = function () {
return this;
};the cache keys are simple strings as expected, probably the cache-handler/redis-strings should return RenderResult not string. Haven't dive into how it works... Please publish this PR to npm so we can play with it easily. Had to publish my own package in npm with this branch, will remove it later |
There was a problem hiding this comment.
@better-salmon Thank you for your hard work on this! I have been playing with this PR locally to have a compatible cache handler for Next15.
I noticed that ROUTE now needs to be APP_ROUTE.
| @@ -188,6 +188,7 @@ export async function registerInitialCache(CacheHandler: CacheHandlerType, optio | |||
| await cacheHandler.set( | |||
| cachePath, | |||
| { | |||
| // @ts-expect-error | |||
| kind: 'ROUTE', | |||
There was a problem hiding this comment.
I believe this needs to be APP_ROUTE now, it seems ROUTE has been deprecated. Metadata routes like favicon.ico or manifest were failing due to this.
|
Hey y'all, any update on this? I'm assuming this resolves the following error
Is there any workaround in the meantime, or does this PR need to be merged? |
|
Following up on this? Anything I can do to help, or are we just waiting on official approval? |
|
Hello, any timeline for full next 15 support? (This PR merged @better-salmon @justinadkins |
|
Based on what I have investigated, there is still a lot to do in this PR. Especially as it is supposed to be backward compatible. There has been really a lot of changes in next 15 in terms of cache. I think better-salomon will need more time to complete this. My approach was to create a simpler version of cache handler that has the minimum features I need. Works for me well, so far, thus I would suggest to bridge the gap in your own capacity and give better-salomon time to do this right |
I believe it is not supposed to be backwards compatible? As per the official docs page |
|
@astrodomas I'm not a contributor, just another dev looking for Next 15 support. I tinkered with this PR a bit though and have been running a patched version of this lib on a low traffic environment without any issues yet. The only changes I made to this PR can be seen here. I also added this to the top of my // @see https://github.com/caching-tools/next-shared-cache/pull/846#issuecomment-2481276786
// eslint-disable-next-line @typescript-eslint/no-explicit-any
String.prototype.toUnchunkedString = function () {
return this;
}; |
|
For anyone following along, I discovered an issue with my fork of this PR today and addressed it with this commit. It came to light when I noticed that an intercepting route wasn't working. |
Next.js is still on v14 due to @neshca/cache-handler: caching-tools/next-shared-cache#846
|
hi 👋 I see this PR has been stalled for a while. We're are currently implementing shared caching with next 15 and would love this update 👀 |
|
Same for us - we are waiting for this missing part to upgrade our apps to next15 - is there anything one can do to help with the development of this change? 😊 |
|
@better-salmon I'd be happy to help get this over the finish line as well. I've been running a forked and patched version of this PR for a little over a month now. I can make a PR against this branch if that would be helpful. |
|
@justinadkins @better-salmon (if you still here) seems like the best solution for node.js + redis (strings) should look like this /**
* @param {string} key
* @param {string[]} [tags]
* @returns {string}
*/
const generateCacheKey = (key, tags = []) => {
const tagsStr = tags.length > 0 ? `:t_${tags.join("t_")}` : "";
return `${key}${tagsStr}:${buildId}`;
};
// set
async set(key, data, ctx) {
const generatedKey = generateCacheKey(key, ctx.tags);
// ...
if (data.revalidate) {
await client.setEx(generatedKey, data.revalidate, value);
} else {
await client.set(generatedKey, value);
}
// ...
}
// get
async get(key, ctx) {
const generatedKey = generateCacheKey(key, ctx.tags);
// ...
if (data.revalidate) {
await client.setEx(generatedKey, data.revalidate, value);
} else {
await client.set(generatedKey, value);
}
// ...
}
/**
* @param {string} pattern
* @param {ReturnType<typeof createClient>} client
*/
export async function deleteKeysWithPattern(pattern, client) {
const script = `
local key = tostring(KEYS[1]);
local delsCount = 0;
local cursor = 0;
repeat
local result = redis.call('SCAN', cursor, 'MATCH', key)
for _, key in ipairs(result[2]) do
redis.call('UNLINK', key);
delsCount = delsCount + 1;
end
cursor = tonumber(result[1]);
until cursor == 0;
return delsCount;`;
try {
const delsCount = await client.eval(script, { keys: [pattern] });
} catch (error) {
console.error(`Error deleting keys matching pattern '${pattern}':`, error);
}
}
async revalidateTag(tags) {
// ...
for await (let tag of tags) {
await deleteKeysWithPattern(`*t_${tag}*`, client);
}
// ...
}great read/write performance, no complexity with sets/hashes/etc, also AWS Elasticache supports EVAL I wrote it today, tested and all works well And do we really need to diff (kinds) APP_ROUTE, FETCH, etc? |
|
Yes, because you have many different things you can cache: fetch, rendered html etc.
|
@justinadkins Would you mind publishing the fork as a temporary measure? |
what is the difference? just store the value, is always string @KraKeN-47 |
|
@jdpnielsen I can get it published soon. Will report back here when I do. @studentIvan I'd need to revisit the source code to see if the additional complexity in this repo is beneficial / necessary compared to your approach. I was just focused on getting this repo working when I was tinkering with it as opposed to home brewing a solution. |
|
@justinadkins it seems like any fix can't fix the pipeTo problem. Because some keys (technically it's ?_rsc=xxx pages cache prefetched from desktop Link component) have rscData Buffer with too much weight now, like 550kb+. pipeTo fails because it has a data read/write timeout it seems (not sure from which library exactly) First Idea I got is simple store big rscData values in LRU cache and check it first, before redis, but here we getting another problem - revalidation of these values. Maybe next.js devs left this "bug" or "feature" for us, to move us to vercel... |
maybe compression is needed? |
How about relying on LRU for some cache values, but have the cache manifest in Redis (shared) so revalidation will apply to all instances. |
Yes can help but we can lost some time and cpu in this case, any idea how to implement it quickly? |
Didn't get you. Can you explain or add details pls? |
I don't know how best to solve the size issue for certain cache values in Redis. We are running Cloud Run instances with a Cloud Storage bucket volume mount on Having this shared cache is great since when scaling up, the new instances can use the same shared data cache. If that tagsManifest could be shared as well (in Redis for example) at least you have a shared invalidation source. At that point it wouldn't matter if you are getting your cache value from Redis, disk or in-memory; Invalidations will be shared anyway. |
|
I found this repo while looking for alternatives: @AyronK is your fork running OK with Next.js 15? I see you made couple of patches in there. |
|
@taraspos yes, we're using it with Next 15 with a bit of workaround https://github.com/fortedigital/nextjs-cache-handler/blob/ce977bd5c45f6f117ec0dad4cc21263ec3b9fae0/packages/nextjs-cache-handler/src/handlers/buffer-string-decorator.ts. However I haven't seen this PR before, not sure how it's gonna work in the next version of this package. |
|
Hey y'all, sorry for the delay in following up. I've published my patch here if you need something temporarily (use at your own discretion). It uses If you'd like to see the diff from this PR to what is published, you can find it here. |
@ts-expect-errorremovedSummary by CodeRabbit
Release Notes
New Features
CacheStateWatchercomponent to monitor and display cache states dynamically.RestartButtoncomponent for restarting the application from the UI.Improvements
Tests