Update README and documentation and the default expire age calculation #879
Conversation
…ity and default expire age calculation adjustments
🦋 Changeset detectedLatest commit: aa2e3e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes in this pull request involve updates to the Changes
Poem
Warning Rate limit exceeded@better-salmon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: assertive
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
auto_title_placeholder: '@coderabbitai'
review_status: true
commit_status: true
poem: true
collapse_walkthrough: false
sequence_diagrams: true
changed_files_summary: true
labeling_instructions: []
path_filters: []
path_instructions: []
abort_on_close: true
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
tools:
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
chat:
auto_reply: true
knowledge_base:
opt_out: false
learnings:
scope: auto
issues:
scope: auto
jira:
project_keys: []
linear:
team_keys: []
pull_requests:
scope: auto
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (3)
docs/cache-handler-docs/src/pages/usage/development-environment.mdx (1)
Line range hint
1-15: Consider enhancing the configuration exampleThe code example effectively shows both configuration approaches, but could benefit from additional context about environment-specific considerations.
Consider adding:
- A note about why the cache handler is specifically needed in production
- Examples of environment-specific configuration using
.envfiles```js filename="next.config.js" copy +// Import configuration from environment variables +const ENABLE_CACHE_HANDLER = process.env.ENABLE_CACHE_HANDLER === 'true'; + const nextConfig = { cacheHandler: process.env.NODE_ENV === 'production' ? require.resolve('./cache-handler.mjs') : undefined, // Use `experimental` option instead of the `cacheHandler` property when using Next.js versions from 13.5.x to 14.0.4 /* experimental: { incrementalCacheHandlerPath: process.env.NODE_ENV === 'production' ? require.resolve('./cache-handler.mjs') : undefined, }, */ + + // Additional production-specific configurations can be added here };docs/cache-handler-docs/src/pages/installation.mdx (1)
Line range hint
103-108: Enhance the configuration note clarityThe note about using the experimental option is important but could be more explicit about version-specific behavior.
Consider updating the comment to:
- // Use `experimental` option instead of the `cacheHandler` property when using Next.js versions from 13.5.x to 14.0.4 + // IMPORTANT: For Next.js versions 13.5.x through 14.0.4, use the `experimental.incrementalCacheHandlerPath` + // option instead of the `cacheHandler` property. For newer versions, use `cacheHandler` as shown above. /* experimental: { incrementalCacheHandlerPath: process.env.NODE_ENV === 'production' ? require.resolve('./cache-handler.mjs') : undefined, }, */packages/cache-handler/src/cache-handler.ts (1)
Line range hint
1-1000: Excellent architectural design!The implementation demonstrates strong adherence to SOLID principles with:
- Clear separation of concerns
- Extensible handler interface
- Comprehensive error handling
- Well-documented public API
Consider adding metrics collection points for cache performance monitoring, which could help validate the new expiration age calculation's impact on cache efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.changeset/flat-boxes-build.md(1 hunks)README.md(1 hunks)docs/cache-handler-docs/src/pages/api-reference/ttl-parameters.mdx(1 hunks)docs/cache-handler-docs/src/pages/installation.mdx(2 hunks)docs/cache-handler-docs/src/pages/usage/development-environment.mdx(1 hunks)packages/cache-handler/README.md(1 hunks)packages/cache-handler/package.json(1 hunks)packages/cache-handler/src/cache-handler.ts(1 hunks)packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
.changeset/flat-boxes-build.md
5-5: null
First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (14)
.changeset/flat-boxes-build.md (1)
1-3: 🧹 Nitpick (assertive)
Verify version bump classification
The change includes a behavioral modification to estimateExpireAge which could affect existing applications. Please confirm if this should be a "major" version bump instead of "minor" according to semver, as it changes the default behavior.
docs/cache-handler-docs/src/pages/usage/development-environment.mdx (1)
Line range hint 10-14: Verify Next.js version compatibility range
The comment specifies compatibility for "Next.js versions from 13.5.x to 14.0.4". Let's verify this version range to ensure accuracy.
packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts (3)
Line range hint 24-37: LGTM! Validation wrapper remains robust
The existing validation logic in createValidatedAgeEstimationFunction properly handles the new calculation method, ensuring:
- Results are properly floored to integers
- Values are capped at MAX_INT32
- Positive integer validation
14-14: Verify potential integer overflow scenarios
While the validation wrapper ensures the final value doesn't exceed MAX_INT32, very large staleAge values could lead to unexpected results due to floating-point multiplication before validation.
✅ Verification successful
Let me gather more information about the validation and bounds checking.
Integer overflow concerns are properly handled
The codebase already implements robust validation through createValidatedAgeEstimationFunction which:
- Uses
Math.floor()to handle floating-point results - Enforces
MAX_INT32(2^31 - 1) as an upper bound - Has test coverage specifically for overflow scenarios
Looking at the usage patterns:
- Default multiplier of 1.5x in the main implementation
- Test scenarios cover edge cases with
MAX_INT32 - Cache testing app shows practical usage with reasonable TTL values (60 seconds)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for staleAge assignments or configurations in the codebase
# to verify typical values and potential upper bounds
rg -g '!*.{lock,md}' --type ts --type js -A 3 'staleAge.*='
Length of output: 1352
Script:
#!/bin/bash
# Search for validation logic and MAX_INT32 usage
ast-grep --pattern 'function createValidatedAgeEstimationFunction($_) {
$$$
}'
# Also check for any constants or validation related to age limits
rg -g '!*.{lock,md}' --type ts --type js "(MAX|max).*(AGE|INT|SECONDS)"
Length of output: 2319
Line range hint 1-37: Verify test coverage and documentation updates
This change to expire age calculation represents a significant behavioral change that could impact cache performance and resource usage.
✅ Verification successful
Let me check the content of the documentation and test files to verify the coverage.
Let me check for specific test cases for the validation functionality.
Test coverage and documentation are comprehensive
The changes are well-documented and thoroughly tested:
- Documentation in
ttl-parameters.mdxclearly explains the expire age calculation and its relationship with stale age - Comprehensive test suite covers all edge cases including:
- Basic age estimation
- Handling of negative values
- Floating point numbers
- MAX_INT32 limits
- Non-numeric inputs
- Error propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related test coverage and documentation updates
echo "Checking for related tests..."
rg -g '*.test.{ts,js}' --type ts --type js -A 5 'getInitialExpireAge|estimateExpireAge'
echo "Checking for documentation updates..."
fd -e md -e mdx | xargs rg -l 'expire.*age|stale.*age'
Length of output: 367
Script:
#!/bin/bash
# Check test coverage and documentation content
echo "Checking test file content..."
ast-grep --pattern 'describe($_, () => {
$$$
test($_, () => {
$$$
getInitialExpireAge($$$)
$$$
})
$$$
})'
echo "Checking documentation content..."
cat docs/cache-handler-docs/src/pages/api-reference/ttl-parameters.mdx
Length of output: 3004
Script:
#!/bin/bash
# Check for validation tests
echo "Checking for validation test cases..."
rg -g '*.test.{ts,js}' --type ts --type js -A 10 'createValidatedAgeEstimationFunction'
echo "Checking for any test files in the same directory..."
fd -e test.ts -e test.js . packages/cache-handler/src/helpers/
Length of output: 9235
docs/cache-handler-docs/src/pages/api-reference/ttl-parameters.mdx (1)
20-20: LGTM! Clear documentation of the default implementation.
The documentation clearly specifies the new default implementation for estimateExpireAge.
packages/cache-handler/package.json (2)
95-95: LGTM! Explicit version constraint improves clarity.
The updated peer dependency constraint ">= 13.5.1 < 15" is more precise and aligns well with the documented support for Next.js versions 13.5.x and 14.x.x. This helps prevent potential compatibility issues with future major versions.
95-95: Verify compatibility with supported Next.js versions
Let's ensure the codebase is compatible with the specified Next.js version range.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Next.js version compatibility verified
The codebase shows proper compatibility with the specified Next.js version range (13.5.1 to <15):
- Uses stable Next.js APIs across the codebase (router, cache, server components)
- Handles version-specific differences (e.g.,
experimentalconfig for 13.5.x to 14.0.4) - No usage of deprecated or removed APIs
- Proper version tags in package distribution (
next13.5,next14) - Clear documentation about version-specific configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Next.js version-specific API usage and compatibility
# Look for Next.js imports and API usage
echo "Checking Next.js API usage patterns..."
rg -A 2 "from 'next|from \"next"
# Look for version-specific comments or conditions
echo "Checking for version-specific code..."
rg -i "next(.js)?\s*(13|14|15)"
# Check for any experimental features usage
echo "Checking experimental features usage..."
rg "experimental"
Length of output: 18471
README.md (2)
30-30: LGTM! Version requirement update is clear and consistent.
The updated Next.js version requirement (13.5.x or 14.x.x) provides clear compatibility guidance for users.
Line range hint 1-999: Consider documenting the expire age calculation changes.
The PR title mentions changes to "default expire age calculation", but this isn't reflected in the README. Consider adding a note about this change if it affects user configuration or behavior.
Let's check if this calculation is documented elsewhere:
docs/cache-handler-docs/src/pages/installation.mdx (2)
10-10: LGTM: Clear version compatibility statement
The Next.js version requirement is clearly stated and matches the version range mentioned in the PR summary.
Let's verify this matches the peer dependencies in package.json:
✅ Verification successful
Next.js version compatibility is correctly documented
The documentation stating "Version 13.5.x or 14.x.x" aligns with the package's peer dependency requirement of ">= 13.5.1 < 15" found in the package.json files. This provides accurate version compatibility information to users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the documented Next.js version range matches package.json
rg -A 2 '"next":\s*"[^"]*"' package.json
Length of output: 42
Script:
#!/bin/bash
# Let's try to find package.json files in the repository
fd package.json
# After finding the files, let's check their content for Next.js dependency
fd package.json --exec cat {} \; | grep -A 2 '"next":'
Length of output: 794
Line range hint 115-134: Well-documented cache population feature
The new section about populating cache with prerendered pages is well-structured and includes important implementation details:
- Clear explanation of why this is needed
- Proper runtime environment check
- Good practice of using dynamic imports
- Helpful callout about Node.js runtime requirement
Let's verify the instrumentation hook feature is properly documented:
✅ Verification successful
Let me check the content of the other documentation file to ensure the instrumentation hook feature is properly documented across the codebase.
Instrumentation hook feature is comprehensively documented
The feature is thoroughly documented across two files with complementary information:
- Installation page covers the basic setup
- Dedicated "Populating Cache on Start" page provides:
- Experimental feature notice and configuration
- Extended usage examples with options
- Standalone output mode considerations
- Monorepo setup instructions
- Relevant external documentation links
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any additional instrumentation hook documentation
rg -l 'instrumentationHook|registerInitialCache' docs/
Length of output: 177
Script:
#!/bin/bash
# Check the content of the other documentation file
cat docs/cache-handler-docs/src/pages/usage/populating-cache-on-start.mdx
Length of output: 3605
packages/cache-handler/src/cache-handler.ts (2)
171-171: Documentation update looks good!
The updated default implementation is clearly documented and aligns with the new behavior.
171-171: Verify cache memory usage impact
The change to multiply staleAge by 1.5 provides a good buffer between stale and expire states, which can help reduce cache churn. However, this could potentially increase memory usage due to longer cache retention.
Let's analyze the cache usage patterns:
packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (2)
docs/cache-handler-docs/src/pages/installation.mdx (2)
Line range hint
124-134: Enhance the instrumentation code example with error handling and path clarificationThe instrumentation code example could benefit from:
- Error handling for dynamic imports
- Clearer path resolution explanation
- Performance impact note
Consider updating the example:
export async function register() { if (process.env.NEXT_RUNTIME === 'nodejs') { - const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); - // Assuming that your CacheHandler configuration is in the root of the project and the instrumentation is in the src directory. - // Please adjust the path accordingly. - // CommonJS CacheHandler configuration is also supported. - const CacheHandler = (await import('../cache-handler.mjs')).default; - await registerInitialCache(CacheHandler); + try { + const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); + // Import the cache handler using a path relative to your project structure + // Example: If instrumentation.ts is in src/instrumentation.ts + // and cache-handler.mjs is in the root, use '../cache-handler.mjs' + const CacheHandler = (await import('../cache-handler.mjs')).default; + console.log('Starting initial cache population...'); + await registerInitialCache(CacheHandler); + console.log('Initial cache population completed'); + } catch (error) { + console.error('Failed to initialize cache:', error); + // Optionally re-throw if you want to fail the startup + throw error; + } } }Also consider adding a note about performance:
> Note: Initial cache population may impact application startup time depending on the number and size of prerendered pages. Monitor your deployment metrics to ensure acceptable startup times.
Line range hint
89-103: Clarify version-specific configuration optionsThe configuration example would benefit from explicit version ranges for each option. Consider adding a table or structured comments:
const nextConfig = { + // For Next.js >= 14.0.5 cacheHandler: process.env.NODE_ENV === 'production' ? require.resolve('./cache-handler.mjs') : undefined, - // Use `experimental` option instead of the `cacheHandler` property when using Next.js versions from 13.5.1 to 14.0.4 - /* experimental: { + // For Next.js 13.5.1 to 14.0.4 + /* experimental: { incrementalCacheHandlerPath: process.env.NODE_ENV === 'production' ? require.resolve('./cache-handler.mjs') : undefined, }, */ experimental: { - // This is required for the experimental feature of pre-populating the cache with the initial data + // Required for cache pre-population (Next.js >= 13.5.1) instrumentationHook: true, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
README.md(1 hunks)docs/cache-handler-docs/src/pages/installation.mdx(1 hunks)docs/cache-handler-docs/theme.config.jsx(1 hunks)packages/cache-handler/README.md(2 hunks)
🔇 Additional comments (2)
docs/cache-handler-docs/theme.config.jsx (1)
46-46: LGTM: Version key updated correctly
The banner key has been properly updated to reflect the new version 1.9.0.
packages/cache-handler/README.md (1)
53-53: LGTM! Version requirements are now precise and consistent.
The Next.js version requirement now:
- Uses precise semver syntax
- Aligns with package.json
- Clearly defines both minimum and maximum versions
better-salmon
force-pushed
the
prepare-for-a-split
branch
from
3688a4a to
702b9ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
README.md(1 hunks)docs/cache-handler-docs/src/pages/installation.mdx(1 hunks)docs/cache-handler-docs/theme.config.jsx(1 hunks)packages/cache-handler/README.md(2 hunks)
🔇 Additional comments (4)
docs/cache-handler-docs/theme.config.jsx (1)
46-46:
Version number mismatch detected
The banner key indicates version 1.9.0, but this conflicts with package.json which shows version 1.8.1.
Let's verify the version across the project:
README.md (1)
30-30: Previous review comment about migration notes is still applicable
The suggestion to add migration notes explaining the Next.js version 15 limitation remains valid.
packages/cache-handler/README.md (1)
53-53: LGTM! Clear and precise version requirement
The version requirement is now more explicit with both lower and upper bounds, which helps users better understand compatibility constraints.
docs/cache-handler-docs/src/pages/installation.mdx (1)
9-11: Verify the minimum version requirements
Let's verify if these version requirements align with the latest LTS releases and package compatibility.
✅ Verification successful
Version requirements are accurate and aligned with package dependencies
The version requirements in the documentation accurately reflect the package's peer dependencies:
- Next.js requirement "13.5.1 or newer (below 15.0.0)" matches the peer dependency ">= 13.5.1 < 15"
- Node.js 18.17 is a reasonable minimum as it's an LTS version
- Redis 4.6.0 is a stable minimum version, well below the current releases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest LTS versions and package compatibility
# Check latest Node.js LTS version
curl -s https://nodejs.org/dist/index.json | jq -r 'map(select(.lts != false)) | .[0].version'
# Check Next.js releases
curl -s https://api.github.com/repos/vercel/next.js/releases | jq -r '.[0].tag_name'
# Check Redis releases
curl -s https://api.github.com/repos/redis/redis/releases | jq -r '.[0].tag_name'
# Check package.json peer dependencies
fd -t f "package.json" -x jq -r '.peerDependencies | select(."next" != null) | ."next"'
Length of output: 405
better-salmon
force-pushed
the
prepare-for-a-split
branch
from
702b9ab to
aa2e3e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
README.md(1 hunks)docs/cache-handler-docs/src/pages/installation.mdx(1 hunks)docs/cache-handler-docs/theme.config.jsx(1 hunks)packages/cache-handler/README.md(2 hunks)
🔇 Additional comments (5)
docs/cache-handler-docs/theme.config.jsx (1)
46-50: LGTM! Clear and accurate version compatibility messaging.
The banner message effectively communicates:
- Current version release (1.9.0)
- Explicit version support range (Next.js 13.5.1-14.x)
- Clear future compatibility requirements (Next.js 15 for version 2.0.0)
Note: This implementation addresses the concerns raised in the previous review comment about version compatibility clarity.
README.md (1)
30-30: Skip previously suggested migration notes.
A previous review comment already suggested adding migration notes for the version constraint.
packages/cache-handler/README.md (3)
11-12: LGTM! Clear announcement of version 1.9.0 changes.
The announcement clearly explains the modification to the estimateExpireAge function and its alignment with Next.js behavior.
13-13: Consider making the deprecation notice more prominent.
The announcement about discontinuing support for Next.js 13 and 14 in version 2.0.0 is significant.
The previous suggestion to add a prominent warning box is still valid. Consider applying the suggested changes from the previous review to help users prepare for this breaking change.
53-53: LGTM! Version requirement is clear and consistent.
The Next.js version requirement is well-formatted and explicitly states the upper bound limit, which aligns with the package.json configuration.
Summary by CodeRabbit
Release Notes
New Features
estimateExpireAgefunction to calculate expiration as(staleAge) => staleAge * 1.5.Documentation
>= 13.5.1 < 15).Bug Fixes
estimateExpireAgemethod in the documentation.