Feat/shlink filters #80
Conversation
godchiSymbionix
requested review from
amalessid,
yassinedorbozgithub and
jacob-khoza-symb
WalkthroughThis pull request introduces enhancements to the share link API by adding a new boolean property, "active," to the schema in Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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.
Actionable comments posted: 13
Outside diff range and nitpick comments (18)
src/usecases/shlinks/get-shlink.ts (2)
2-2: Consider removing unused imports.The imports
FilterandQueryFilterare not used in the visible code. Consider removing them if they're not needed elsewhere in the file.-import { Filter, QueryFilter, LogicOperator } from '@/infrastructure/repositories/interfaces/repository.interface'; +import { LogicOperator } from '@/infrastructure/repositories/interfaces/repository.interface';
21-24: LGTM: Repository query updated correctly.The
findManymethod call has been correctly updated to include thelogicOperator, allowing for status-based filtering while maintaining theuser_idfilter.Consider adding a space after the colon in the
user_idproperty for consistent formatting:const entities = await context.repo.findMany({ - user_id:data.user_id, + user_id: data.user_id, ...logicOperator });src/app/api/v1/users/[id]/route.ts (2)
45-45: Consider documenting the need for both dynamic rendering and unstable_noStoreThe
unstable_noStore()call here prevents caching of the response, which is consistent with thedynamic = 'force-dynamic'export. However, these two approaches might be redundant.Consider adding a comment explaining why both are necessary, or if one approach would suffice. If both are required, document the specific scenarios or edge cases that necessitate this double layer of cache prevention.
Line range hint
1-56: Overall changes enhance dynamism but may impact performanceThe changes in this file focus on preventing caching and ensuring dynamic rendering of the user retrieval route. This aligns well with the PR objectives of implementing filters for share links, as it ensures that the most up-to-date user data is always fetched.
However, it's important to consider the performance implications of these changes:
- Preventing all caching might increase response times and server load.
- For user data that doesn't change frequently, this level of dynamism might be unnecessary.
Consider implementing a more granular approach to caching and dynamic rendering. For example, you could use stale-while-revalidate strategies or cache with a short TTL for less frequently changing data.
src/app/api/v1/users/[id]/ips/route.ts (2)
50-64: LGTM: Caching prevention and log formatting improvementsThe addition of
unstable_noStore()aligns with the earlier changes to prevent caching. The log statement reformatting improves readability.For consistency, consider applying the same multi-line formatting to the log statement at the end of the function (around line 71).
Apply this formatting to the final log statement for consistency:
logger.log( `Error occurred while retrieving user's patient summary data: ${error}`, );
Line range hint
1-74: Summary of changes and potential impactThe changes in this file focus on two main areas:
- Preventing caching through the use of
unstable_noStoreanddynamic = 'force-dynamic'.- Improving code readability through log statement reformatting.
While these changes enhance the dynamism of the API and improve code quality, they may have performance implications due to the prevention of caching and static optimization. It's important to monitor the performance of this endpoint after these changes and consider if this level of dynamism is necessary for all requests.
Additionally, ensure that these changes are applied consistently across similar API routes in the project for maintainability and predictable behavior.
src/app/api/v1/server-configs/route.ts (2)
47-52: Approved: Changes in POST handlerThe addition of
unstable_noStore()and the formatting improvement in the logger call are appropriate. These changes ensure that the POST request is not cached, which is crucial for maintaining up-to-date server configurations.However, consider moving the
unstable_noStore()call to the beginning of the function for consistency with best practices.Consider applying this minor change:
export async function POST(request: Request) { + unstable_noStore(); let dto: CreateServerConfigDto = await request.json(); logger.info( `Creating server config API with request: ${JSON.stringify(dto)}`, ); try { - unstable_noStore(); await validateUserRoles(request, 'admin'); const model = mapDtoToModel(dto as ServerConfigDto);
80-89: Approved: Changes in GET handlerThe addition of
unstable_noStore()and the formatting improvements in the response construction are appropriate. These changes ensure that the GET request always fetches fresh data, which is crucial for implementing accurate filters.However, consider moving the
unstable_noStore()call to the beginning of the function for consistency with best practices and the suggested change in the POST handler.Consider applying this minor change:
export async function GET(request: Request) { + unstable_noStore(); try { - unstable_noStore(); logger.info(`Getting all available server configs data`); await validateUserRoles(request, 'admin'); const serverConfigs = await getServerConfigsUseCase({ repo }); return NextResponse.json( serverConfigs.map((x) => mapModelToDto(x)), { status: 200 }, ); } catch (error) {src/app/api/v1/share-links/[id]/endpoints/[endpointId]/route.ts (2)
70-72: Approved: Improved logger statement formattingThe reformatting of the logger statement improves readability without changing functionality. It clearly logs all relevant identifiers.
Consider using template literals for even better readability:
logger.info( `Getting an endpoint data with share link id: ${params.id}, endpoint id: ${params.endpointId} and ticket id: ${ticketId}` );
100-102: Approved: Improved logger statement formattingThe reformatting of the logger statement improves readability without changing functionality. It clearly logs all relevant identifiers.
Consider using template literals for even better readability:
logger.info( `Getting an endpoint data of user id: ${shlink.getUserId()} with share link id: ${params.id}, endpoint id: ${params.endpointId} and ticket id: ${ticketId}` );src/usecases/shlinks/get-shlink.test.ts (2)
Line range hint
50-73: LGTM: Comprehensive mock data setup.The additional mock data is well-structured and covers various scenarios, which is excellent for testing the new filtering functionality. This setup will ensure robust testing of the
getSHLinkUseCase.Consider adding a comment explaining the purpose of each mock entity (e.g., "Active, non-expired link" and "Inactive, future expiration link"). This would enhance readability and make it easier to understand the test scenarios at a glance.
115-129: LGTM: Comprehensive test for expired status filter.This test case effectively verifies that the
getSHLinkUseCasecorrectly applies the expired status filter. The use of Jest's fake timers is an excellent approach for testing time-dependent functionality. The test ensures that the repository'sfindManymethod is called with the appropriate parameters when the status is set to "expired".Consider adding a test case for a non-expired link to ensure the filter doesn't incorrectly include future expiration dates. This would provide more comprehensive coverage of the expiration filtering logic.
src/domain/dtos/shlink.ts (1)
Line range hint
63-63: LGTM! Consider adding Swagger documentation for the newactiveproperty.The addition of the
activeproperty to theSHLinkDtoclass aligns well with the PR objectives for implementing share link filters. This will allow for better tracking and filtering of share link statuses.To maintain consistency with the existing documentation, consider adding Swagger documentation for the new
activeproperty. Here's a suggested addition to the Swagger schema:* active: * type: boolean * description: A boolean indicating whether the share link is active. * example: truesrc/app/api/v1/share-links/[id]/route.ts (2)
87-92: LGTM! Consider enhancing error logging.The addition of
unstable_noStore()is appropriate for this dynamic route. The reformatted logging improves readability.Consider adding error logging to capture any potential issues with
request.json():unstable_noStore(); - let { managementToken, passcode, recipient }: SHLinkRequestDto = - await request.json(); + let { managementToken, passcode, recipient }: SHLinkRequestDto; + try { + ({ managementToken, passcode, recipient } = await request.json()); + } catch (error) { + logger.error(`Error parsing request body: ${error}`); + return NextResponse.json({ message: 'Invalid request body' }, { status: 400 }); + }
170-175: LGTM! Consider consistent error handling.The addition of
unstable_noStore()and the reformatted logging are appropriate and improve the code.For consistency with the suggested improvement in the POST handler, consider adding similar error handling for
request.json():unstable_noStore(); - let { managementToken, oldPasscode, passcode, expiryDate }: SHLinkUpdateDto = - await request.json(); + let { managementToken, oldPasscode, passcode, expiryDate }: SHLinkUpdateDto; + try { + ({ managementToken, oldPasscode, passcode, expiryDate } = await request.json()); + } catch (error) { + logger.error(`Error parsing request body: ${error}`); + return NextResponse.json({ message: 'Invalid request body' }, { status: 400 }); + }src/mappers/shlink-mapper.test.ts (3)
175-175: LGTM! Consider adding a test for inactive links.The addition of the
activeproperty in the expected result is correct and aligns with the PR objectives. It properly reflects the active status of the share link in the mapped DTO.To improve test coverage, consider adding a test case for an inactive share link (where
activeisfalse) to ensure the mapper handles both active and inactive states correctly.
263-263: LGTM! Consider adding a test for expired links.The addition of the
activeproperty in this test case is correct and consistent with the previous changes. It ensures that theactivestatus is properly included in the mapped DTO when no ticket is provided.To further improve test coverage and align with the PR objectives, consider adding a test case for an expired share link. This would involve creating a
shlinkModelwith a past expiry date and verifying that themapModelToMiniDtofunction correctly handles this scenario.
Line range hint
175-263: Overall LGTM! Consider expanding test coverage.The changes consistently add the
activeproperty to the expected results ofmapModelToMiniDtotests across different scenarios. This aligns well with the PR objectives of implementing filters based on the status of share links.To further improve the test suite and ensure comprehensive coverage of the new filtering functionality:
- Add a test case for an inactive share link (where
activeisfalse).- Add a test case for an expired share link (with a past expiry date).
- Consider adding test cases that combine different states (e.g., active but expired, inactive and expired) to ensure the mapper handles all possible combinations correctly.
These additions would help validate that the
mapModelToMiniDtofunction correctly handles all the filter categories mentioned in the PR objectives (Expired, Active, and Inactive).
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- src/app/api/route.ts (1 hunks)
- src/app/api/v1/server-configs/route.ts (4 hunks)
- src/app/api/v1/share-links/[id]/accesses/route.ts (4 hunks)
- src/app/api/v1/share-links/[id]/deactivate/route.ts (3 hunks)
- src/app/api/v1/share-links/[id]/endpoints/[endpointId]/route.ts (4 hunks)
- src/app/api/v1/share-links/[id]/endpoints/route.ts (3 hunks)
- src/app/api/v1/share-links/[id]/qrcode/route.ts (3 hunks)
- src/app/api/v1/share-links/[id]/route.ts (4 hunks)
- src/app/api/v1/share-links/route.ts (4 hunks)
- src/app/api/v1/users/[id]/ips/route.ts (3 hunks)
- src/app/api/v1/users/[id]/route.ts (3 hunks)
- src/app/api/v1/users/route.ts (3 hunks)
- src/domain/dtos/shlink.ts (2 hunks)
- src/mappers/shlink-mapper.test.ts (3 hunks)
- src/mappers/shlink-mapper.ts (1 hunks)
- src/usecases/shlinks/get-shlink.test.ts (4 hunks)
- src/usecases/shlinks/get-shlink.ts (1 hunks)
Additional comments not posted (45)
src/usecases/shlinks/get-shlink.ts (2)
8-8: LGTM: Function signature updated correctly.The addition of the optional
statusparameter aligns with the PR objectives and maintains backward compatibility.
19-19: LGTM: Robust logic operator assignment.The assignment of
logicOperatorbased ondata.statusis implemented correctly, handling undefined or invalid status values gracefully.src/app/api/v1/users/[id]/route.ts (3)
43-43: LGTM: Minor syntax improvementThe addition of a semicolon at the end of the logger statement improves code consistency. This change is good for maintaining a uniform coding style.
13-13: Consider the performance implications of force-dynamic renderingSetting
dynamic = 'force-dynamic'ensures that this route is dynamically rendered for every request, which aligns with the use ofunstable_noStore. However, this approach prevents any caching, which could impact performance, especially for frequently accessed user data.Consider if this level of dynamism is necessary for all cases, or if a more granular approach to dynamic rendering could be implemented.
To check the consistency of dynamic rendering across routes, run:
Verification successful
Consistent Use of
force-dynamicConfirmedThe
export const dynamic = 'force-dynamic';statement is consistently used across multiple API route files, including:
src/app/api/v1/users/route.tssrc/app/api/v1/server-configs/route.tssrc/app/api/v1/share-links/route.tssrc/app/api/v1/share-links/[id]/deactivate/route.tssrc/app/api/v1/share-links/[id]/accesses/route.tssrc/app/api/v1/users/[id]/ips/route.tssrc/app/api/v1/share-links/[id]/qrcode/route.tssrc/app/api/v1/share-links/[id]/endpoints/route.tssrc/app/api/v1/share-links/[id]/endpoints/[endpointId]/route.tsThis ensures that all these routes are dynamically rendered per request, aligning with the use of
unstable_noStoreand maintaining consistent behavior across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of force-dynamic rendering across routes # Test: Search for dynamic export in route files rg --type typescript 'export const dynamic = .force-dynamic.' 'src/app/api'Length of output: 110
Script:
#!/bin/bash # Description: Check for consistent usage of force-dynamic rendering across routes # Test: Search for dynamic export in TypeScript route files rg --type ts 'export const dynamic = \'force-dynamic\'' 'src/app/api'Length of output: 93
Script:
#!/bin/bash # Description: Check for consistent usage of force-dynamic rendering across routes # Test: Search for dynamic export in TypeScript route files rg --type ts "export const dynamic = 'force-dynamic'" src/app/apiLength of output: 917
1-1: Be cautious with the use of experimental Next.js featuresThe
unstable_noStorefunction is an experimental feature in Next.js. While it serves the purpose of opting out of caching for specific requests, be aware that its API might change in future releases. Consider adding a comment explaining why this feature is necessary for this route.To ensure this feature is used consistently across the project, run the following command:
Verification successful
'unstable_noStore' is used consistently across the codebase
The usage of
unstable_noStorehas been verified and is consistently applied across multiple route files as intended. Ensure that all instances include comments explaining the necessity of opting out of caching for maintainability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of unstable_noStore across the project # Test: Search for unstable_noStore usage rg --type typescript 'unstable_noStore'Length of output: 74
Script:
#!/bin/bash # Description: Check for consistent usage of unstable_noStore across the project # Search for 'unstable_noStore' in .ts and .tsx files rg 'unstable_noStore' --glob '*.ts' --glob '*.tsx'Length of output: 2170
src/app/api/v1/share-links/[id]/deactivate/route.ts (4)
1-1: LGTM: Import ofunstable_noStoreis appropriate.The addition of
unstable_noStorefrom 'next/cache' is in line with the PR's objective. It ensures that the most current data is fetched when deactivating share links, which is crucial for maintaining accurate link statuses.
13-13: LGTM: Dynamic export enhances real-time data accuracy.The addition of
export const dynamic = 'force-dynamic'ensures that this route is always dynamically rendered. This complements the use ofunstable_noStoreand aligns with the PR's goal of implementing accurate share link filters by ensuring that the most current link status is always reflected.
45-45: LGTM: Proper implementation ofunstable_noStore().The addition of
unstable_noStore()at the beginning of the try block ensures that the entire function execution is not cached. This is consistent with the earlier modifications and aligns perfectly with the PR's objective of implementing accurate share link filters. The core logic for deactivating the share link remains intact, which is correct.
Line range hint
1-56: Summary: Effective implementation of caching prevention for accurate share link filtering.The changes made to this file collectively contribute to preventing caching and ensuring real-time data retrieval when deactivating share links. This is achieved through:
- Importing and using
unstable_noStore.- Setting
dynamic = 'force-dynamic'for the route.- Calling
unstable_noStore()at the beginning of the DELETE function.These modifications align perfectly with the PR's objective of implementing accurate share link filters. By ensuring that the most up-to-date data is always fetched, the system can correctly categorize share links based on their current status (expired, active, or inactive).
The core logic for deactivating share links remains unchanged, which is appropriate. Overall, these changes enhance the reliability and accuracy of the share link management system.
src/app/api/v1/users/route.ts (4)
19-19: Approved: Forcing dynamic rendering for this routeThe
dynamic = 'force-dynamic'export ensures that this route is always dynamically rendered. This is consistent with the use ofunstable_noStorein the POST function and prevents any unintended caching or static optimization.
Line range hint
1-65: Summary: Consistent implementation of dynamic, no-cache approachThe changes in this file consistently implement a dynamic, no-cache approach for the user creation route. This aligns with the PR objectives of enhancing functionality and implementing filters. Key points:
- Use of experimental
unstable_noStorefeature.- Forcing dynamic rendering with
dynamic = 'force-dynamic'.- Opting out of caching in the POST request handler.
These changes ensure that user creation requests are always processed with the most up-to-date data. However, be mindful of potential performance impacts and keep an eye on the stability of the experimental features in future Next.js updates.
53-53: Approved: Opting out of caching for user creationThe addition of
unstable_noStore()ensures that the user creation request is not cached, which is appropriate for a POST request. This is consistent with the dynamic rendering approach used in this route.To monitor the performance impact of this change, consider implementing server-side timing metrics. You can use the following script to check if such metrics are already in place:
#!/bin/bash # Check for existing performance monitoring rg -i 'performance|timing|metric' --type tsIf no results are found, consider adding performance monitoring to track the impact of disabling caching.
1-1: Be aware of using experimental Next.js featureThe
unstable_noStoreimport from 'next/cache' is an experimental Next.js feature. While it's useful for opting out of caching, be aware that its API might change in future versions.To ensure you're using the latest version and to monitor for any breaking changes, run the following command:
Verification successful
Confirmed Use of Experimental Next.js Feature
The project is using Next.js version 14.2.5, which includes the
unstable_noStorefeature. Ensure to monitor Next.js updates for any changes to this experimental API.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the current Next.js version and any warnings about experimental features npm list next npm view next@latestLength of output: 613
Script:
#!/bin/bash # Extract the installed Next.js version from package.json jq -r '.dependencies.next // .devDependencies.next // "Next.js not found"' package.jsonLength of output: 96
src/app/api/v1/users/[id]/ips/route.ts (3)
25-25: LGTM: Logger instantiation refactoringMoving the logger instantiation improves code organization without affecting functionality.
18-18: Verify the necessity of forcing dynamic renderingSetting
dynamic = 'force-dynamic'ensures that this route is always dynamically rendered. While this aligns with the use ofunstable_noStore, it may impact performance by preventing static optimization.Consider if this level of dynamism is necessary for all requests to this endpoint. If not, you might want to explore more granular caching strategies.
To check if this pattern is consistently applied across similar routes, run:
Verification successful
Consistent usage of
dynamic = 'force-dynamic'confirmedThe
dynamic = 'force-dynamic'export is consistently applied across multiple API routes, aligning with the use ofunstable_noStoreto prevent caching. This uniform approach ensures that these endpoints are always dynamically rendered, maintaining up-to-date responses as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent usage of force-dynamic across API routes rg --type typescript "export const dynamic = 'force-dynamic'" -g 'src/app/api/**/*.ts'Length of output: 129
Script:
#!/bin/bash # Check for consistent usage of force-dynamic across API routes using correct file type rg "export const dynamic = 'force-dynamic'" -g 'src/app/api/**/*.ts' -g 'src/app/api/**/*.tsx'Length of output: 946
1-1: Consider the implications of usingunstable_noStoreThe addition of
unstable_noStorefromnext/cacheintroduces an experimental Next.js feature. While this can help prevent caching of responses, be aware that it may lead to potential instability or breaking changes in future Next.js updates.To ensure this import is used consistently across the project, run the following command:
Verification successful
Usage of
unstable_noStoreis consistent across the project.
All instances ofunstable_noStorefromnext/cacheare consistently used across the specified files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent usage of unstable_noStore across the project rg --type typescript "import.*unstable_noStore.*from 'next/cache'" -g '!**/node_modules/**'Length of output: 134
Script:
# List all supported file types in ripgrep rg --type-listLength of output: 4821
Script:
#!/bin/bash # Check for consistent usage of unstable_noStore across the project with correct file type rg --type ts "import.*unstable_noStore.*from 'next/cache'" -g '!**/node_modules/**'Length of output: 1161
src/app/api/v1/share-links/[id]/qrcode/route.ts (5)
48-49: LGTM: Improved function parameter formattingThe separation of the
requestparameter and the destructuredparamsobject onto separate lines improves code readability without altering functionality.
54-57: LGTM: Improved logging format and addition ofunstable_noStore()The logging statement has been reformatted for better readability. The addition of
unstable_noStore()immediately after logging is consistent with the earlier changes and ensures that this route's response is not cached. This is appropriate for a QR code generation endpoint where you want to ensure fresh data.
Line range hint
1-77: Summary: Effective implementation of caching prevention for QR code generationThe changes in this file consistently work towards preventing caching for the QR code generation route. This is achieved through:
- Importing and using
unstable_noStore.- Setting
dynamicexport to 'force-dynamic'.- Calling
unstable_noStore()at the start of the POST function.These modifications ensure that the QR codes generated will always reflect the current state of share links, which aligns well with the PR objectives of implementing share link filters. The code maintains its core functionality while improving its ability to provide up-to-date responses.
13-13: LGTM: Setting dynamic export to 'force-dynamic'The addition of
export const dynamic = 'force-dynamic';ensures that this route is always dynamically rendered. This is consistent with the use ofunstable_noStoreand supports the goal of preventing caching for this QR code generation route.Let's check if this pattern is used consistently across other routes:
#!/bin/bash # Description: Check usage of 'force-dynamic' export across the project rg --type typescript "export const dynamic = 'force-dynamic'"
1-1: LGTM: Addition ofunstable_noStoreimportThe import of
unstable_noStorefrom 'next/cache' is appropriate for managing caching behavior in this route. However, be aware that the "unstable" prefix indicates this API might change in future Next.js versions.To ensure this import is used consistently across the project, let's check its usage:
Verification successful
Verified: Consistent Usage of
unstable_noStoreAcross ProjectAll instances of
unstable_noStoreimported from'next/cache'are used consistently to disable caching in appropriate API routes. This ensures that dynamic or sensitive data is handled correctly without unintended caching.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of unstable_noStore across the project rg --type typescript 'import.*unstable_noStore.*from.*next/cache'Length of output: 104
Script:
#!/bin/bash # Description: Check usage of unstable_noStore across the project with corrected file type rg --type ts 'import.*unstable_noStore.*from.*next/cache'Length of output: 1129
Script:
#!/bin/bash # Description: Extract surrounding lines to understand the context of unstable_noStore usage fd --extension ts | xargs grep -H -C 5 "unstable_noStore"Length of output: 18819
src/app/api/v1/share-links/[id]/accesses/route.ts (4)
1-1: LGTM: Import ofunstable_noStoreis appropriate.The addition of
unstable_noStorefrom 'next/cache' is in line with the PR objectives. This function will help ensure that the API always fetches fresh data, which is crucial for implementing accurate filters based on the current status of share links.
19-19: LGTM: Dynamic rendering enforced.The addition of
export const dynamic = 'force-dynamic'ensures that this route is always dynamically rendered. This complements the use ofunstable_noStoreand aligns with the PR's objective of implementing filters that require current data for share link statuses.
58-60: LGTM: Improved log statement readability.The reformatting of the logging statement enhances code readability without altering the functionality. This change contributes to better code maintainability.
Line range hint
1-84: Summary: Changes effectively support share link filtering objectives.The modifications in this file, including the addition of
unstable_noStoreand settingdynamic = 'force-dynamic', align well with the PR objectives of implementing share link filters. These changes ensure that the API always works with fresh data, which is crucial for accurately determining the status of share links (expired, active, or inactive).The code maintains its original functionality while improving its ability to work with current data. The suggested optimization of consolidating
unstable_noStore()calls would further enhance the code's efficiency.Overall, these changes provide a solid foundation for implementing the desired share link filtering functionality.
src/app/api/v1/server-configs/route.ts (3)
17-17: Approved: Addition of dynamic exportThe addition of
export const dynamic = 'force-dynamic';is appropriate and consistent with the use ofunstable_noStore. This ensures that the route is always dynamically rendered, which is crucial for implementing real-time filters and working with the most up-to-date data.
Line range hint
1-89: Summary: Improvements in server config route handlingThe changes in this file effectively implement dynamic data fetching and rendering, which aligns well with the PR objectives of implementing share link filters. The use of
unstable_noStore()anddynamic = 'force-dynamic'ensures that the API always works with the most up-to-date data, which is crucial for accurate filtering.The code structure, error handling, and logging practices have been maintained, with minor improvements in formatting. The suggested minor adjustments to the placement of
unstable_noStore()calls would further enhance code consistency.To ensure the changes are comprehensive, let's verify if similar changes have been applied to other relevant API routes:
#!/bin/bash # Description: Verify similar changes in other API routes # Test 1: Check for unstable_noStore usage in other API routes echo "Checking for unstable_noStore usage in API routes:" rg --type typescript 'unstable_noStore\(\)' src/app/api # Test 2: Check for dynamic = 'force-dynamic' in other API routes echo "Checking for dynamic = 'force-dynamic' in API routes:" rg --type typescript "export const dynamic = 'force-dynamic'" src/app/api
1-1: Approved: Addition ofunstable_noStoreimportThe import of
unstable_noStorefrom 'next/cache' is appropriate for implementing dynamic data fetching, which aligns with the PR objectives. However, be aware that the 'unstable' prefix indicates this API might change in future Next.js versions.To ensure this import is used correctly, let's verify its usage:
Verification successful
Verified:
unstable_noStoreis correctly usedThe
unstable_noStorefunction is properly utilized insrc/app/api/v1/server-configs/route.tswith two instances found. This ensures that caching is appropriately managed as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of unstable_noStore in the file # Test: Search for unstable_noStore usage rg --type typescript 'unstable_noStore\(\)' src/app/api/v1/server-configs/route.tsLength of output: 443
Script:
#!/bin/bash # Description: Verify the usage of unstable_noStore in the file # Test: Search for unstable_noStore usage with correct type rg --type ts 'unstable_noStore\(\)' src/app/api/v1/server-configs/route.tsLength of output: 124
src/mappers/shlink-mapper.ts (1)
71-71: LGTM! The addition of theactiveproperty aligns with PR objectives.The inclusion of the
activeproperty in theSHLinkMiniDtois a good addition. It directly supports the PR's goal of implementing share link filters based on their status. This change will allow for easier categorization of share links as active or inactive, which is one of the key features mentioned in the PR objectives.src/app/api/v1/share-links/[id]/endpoints/route.ts (4)
1-1: LGTM: Import ofunstable_noStoreis appropriate.The addition of
unstable_noStorefrom 'next/cache' is a good choice for ensuring that the latest data is always fetched, which is crucial for implementing accurate filters for share links based on their current status.
29-29: LGTM: Dynamic rendering is appropriate for this route.The addition of
export const dynamic = 'force-dynamic'ensures that this route is always dynamically rendered. This is consistent with the use ofunstable_noStoreand is appropriate for handling share link filters that require up-to-date data.
72-74: LGTM: Improved log statement formatting.The reformatting of the log statement enhances readability while maintaining the same informational content. This change contributes to better code maintainability.
Line range hint
1-103: Overall assessment: Changes align well with PR objectives.The modifications in this file, including the addition of
unstable_noStoreand settingdynamic = 'force-dynamic', are consistent with the PR's goal of implementing share link filters. These changes ensure that the latest data is always used, which is crucial for accurate filtering based on share link status (expired, active, inactive).The code changes are well-implemented and contribute to the enhancement of the share links functionality. The suggested minor improvement regarding the placement of
unstable_noStore()would further solidify the consistency of the no-cache behavior.src/app/api/v1/share-links/[id]/endpoints/[endpointId]/route.ts (3)
27-27: Approved: Setting dynamic export to 'force-dynamic'The addition of
export const dynamic = 'force-dynamic';ensures that this route is always dynamically rendered. This is consistent with the use ofunstable_noStoreand supports the implementation of accurate filters based on current share link statuses.This setting guarantees that each request to this endpoint will fetch fresh data, which is crucial for determining the current state (expired, active, or inactive) of share links.
Line range hint
1-110: Summary: Changes align with PR objectivesThe modifications in this file contribute to the implementation of share link filters by ensuring fresh data is fetched on each request. This is crucial for accurately determining the status of share links (expired, active, or inactive) based on current data.
The addition of
unstable_noStore()and settingdynamic = 'force-dynamic'work together to prevent caching and ensure dynamic rendering. This approach supports the PR's goal of implementing filters based on the current status of share links.While the core functionality aligns well with the PR objectives, consider the suggested minor improvements in code style and the placement of
unstable_noStore()to further enhance the implementation.
1-1: Approved: Addition ofunstable_noStoreimportThe import of
unstable_noStorefrom 'next/cache' is appropriate for implementing dynamic data fetching, which aligns with the PR objectives. However, be aware that this is an experimental feature in Next.js and may be subject to changes in future versions.To ensure this feature is used consistently across the project, let's check its usage:
Verification successful
Usage of
unstable_noStoreis consistent across the codebase.
The search results show thatunstable_noStoreis appropriately imported and used in multiple TypeScript files, aligning with existing project practices. No inconsistencies or issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of unstable_noStore rg --type typescript 'unstable_noStore'Length of output: 74
Script:
#!/bin/bash # Search for other usages of unstable_noStore in TypeScript files rg 'unstable_noStore' --type ts --type tsxLength of output: 70
Script:
#!/bin/bash # Search for other usages of unstable_noStore in .ts and .tsx files fd -e ts -e tsx -x rg 'unstable_noStore' {}Length of output: 987
src/mappers/shlink-mapper.test.ts (1)
215-215: LGTM! Consistent update for the no-files test case.The addition of the
activeproperty in this test case is correct and consistent with the previous change. It ensures that theactivestatus is properly included in the mapped DTO even when no files are provided.src/app/api/v1/share-links/route.ts (1)
87-87: Ensure proper handling ofstatusingetSHLinkUseCaseVerify that the
statusparameter passed togetSHLinkUseCaseis correctly handled within the use case, especially after adding validation to thestatusparameter. This ensures that only appropriate share links are retrieved.Run the following script to check the implementation of
getSHLinkUseCase:This script searches for the
getSHLinkUseCasefunction implementation to review how thestatusparameter is utilized.
src/app/api/v1/share-links/[id]/endpoints/[endpointId]/route.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/app/api/v1/share-links/route.ts (4 hunks)
Additional comments not posted (5)
src/app/api/v1/share-links/route.ts (5)
78-84: LGTM: Proper parameter extraction and validationThe changes to extract and validate the 'status' query parameter are well-implemented. The code properly handles case-insensitivity, validates against a predefined list of valid statuses, and returns an appropriate error response for invalid inputs.
92-92: LGTM: Proper integration of status filterThe modification to include the
statusparameter in thegetSHLinkUseCasecall is correct and aligns with the PR objectives. This change enables filtering of share links based on their status.
18-18: Remove redundantdynamicexportWhen using
Cache-Controlheaders to control caching, theexport const dynamic = 'force-dynamic';line becomes redundant.Apply this diff to remove the redundant export:
-export const dynamic = 'force-dynamic';Likely invalid or redundant comment.
49-51: Remove sensitive information from log statementLogging user identifiers such as
dto.userIdanddto.namecan expose personally identifiable information (PII) and pose security risks.Apply this diff to remove PII from the log statement:
logger.info( - `Creating a share link API with parameters, ${JSON.stringify({ name: dto.name, userId: dto.userId })}`, + 'Creating a new share link', );Likely invalid or redundant comment.
87-87: Replaceunstable_noStore()withCache-ControlheaderAs with the POST method, replace the
unstable_noStore()call with aCache-Controlheader in the response for stable caching behavior.Apply this diff to adjust the caching strategy:
export async function GET(request: Request) { // ... existing code ... try { - unstable_noStore(); const { id } = await getUserProfile(request); // ... existing code ... return NextResponse.json( newShlink.map((shlink) => mapModelToMiniDto(shlink)), - { status: 200 }, + { + status: 200, + headers: { 'Cache-Control': 'no-store' }, + }, ); } catch (error) { return handleApiValidationError(error, logger); } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (1)
src/app/api/v1/share-links/route.ts (1)
78-79: LGTM! Consider adding input validationThe extraction and processing of the 'status' query parameter look good. Converting to lowercase ensures consistency.
Consider adding input validation for the 'status' parameter to ensure it only accepts expected values. This can be done right after these lines.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- public/swagger.json (1 hunks)
- src/app/api/v1/share-links/route.ts (4 hunks)
Additional comments not posted (4)
src/app/api/v1/share-links/route.ts (3)
81-84: LGTM! Good input validationThe validation of the 'status' parameter is well-implemented. It checks against a predefined list of valid statuses and returns an appropriate error response if the input is invalid.
Line range hint
1-92: Summary of changes and final recommendationsThe changes in this file enhance the share links API by adding status-based filtering. This is a valuable addition that improves the API's functionality. However, there are a few key points to address:
- Remove the use of
unstable_noStore()and thedynamicexport.- Implement
Cache-Controlheaders in both POST and GET methods for consistent caching behavior.- Avoid logging sensitive user information in the POST method.
- The input validation for the 'status' parameter in the GET method is well-implemented.
Once these points are addressed, the changes will significantly improve the API's functionality and security.
49-51: Avoid logging sensitive user informationLogging user identifiers such as
dto.userIdanddto.namecan expose personally identifiable information (PII) and pose security risks. Consider omitting or masking sensitive data in logs.Apply this diff to remove PII from the log statement:
logger.info( - `Creating a share link API with parameters, ${JSON.stringify({ name: dto.name, userId: dto.userId })}`, + 'Creating a new share link.', );Likely invalid or redundant comment.
public/swagger.json (1)
272-276: LGTM! The "active" property is a valuable addition.The new "active" property in the SHLinkMini schema is well-defined and aligns with the PR objectives. It will enable filtering of share links based on their active status.
src/app/api/v1/share-links/route.ts
Outdated
| const { id } = await getUserProfile(request); | ||
|
|
||
| logger.info(`Getting all share links by user with user id: ${id}`); | ||
|
|
||
| const newShlink = await getSHLinkUseCase({ repo }, { user_id: id }); | ||
| const newShlink = await getSHLinkUseCase({ repo }, { user_id: id, status:status }); |
There was a problem hiding this comment.
just have status
src/app/api/v1/share-links/route.ts
Outdated
| const { searchParams } = new URL(request.url) | ||
| let status: string | null = searchParams.get('status')?.toLowerCase() || null | ||
|
|
||
| const validStatuses = ['expired', 'active', 'inactive']; |
There was a problem hiding this comment.
move the validation logic to a place other than the controller
src/mappers/shlink-mapper.test.ts
Outdated
| @@ -211,6 +212,7 @@ describe('mapModelToMiniDto', () => { | |||
| passwordRequired: true, | |||
| name: 'name', | |||
| expiryDate: date, | |||
| active:true, | |||
There was a problem hiding this comment.
fix the spacing, run yarn lint:fix
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
src/app/utils/error-handler.ts (2)
24-28: LGTM: New error handling for ParameterValidationErrorThe new conditional branch for handling
ParameterValidationErroris well-implemented and consistent with the existing error handling pattern. It correctly returns a JSON response with the appropriate error details and status code.Minor formatting suggestion:
- }else if (error instanceof ParameterValidationError) { + } else if (error instanceof ParameterValidationError) {Add a space before
elsefor consistency with other conditional statements in the file.
Line range hint
1-50: Overall: Good addition to error handling logicThe new error handling for
ParameterValidationErroris a valuable addition to the existing error handling logic. It enhances the system's ability to provide specific error information for parameter validation issues while maintaining consistency with the existing error handling pattern.Consider extracting the common response structure into a helper function to reduce code duplication. For example:
function createErrorResponse(error: string, detail: any, status: number) { return NextResponse.json({ error, detail }, { status }); }This could be used throughout the function to simplify the code:
if (error instanceof ParameterValidationError) { return createErrorResponse(BAD_REQUEST, error, 422); }This refactoring would make the code more DRY and easier to maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/app/api/v1/share-links/route.ts (5 hunks)
- src/app/utils/error-handler.ts (2 hunks)
- src/app/utils/validate.ts (1 hunks)
- src/usecases/shlinks/get-shlink.ts (1 hunks)
Additional comments not posted (12)
src/app/utils/validate.ts (3)
1-1: LGTM: Correct import statementThe import of
zfrom the 'zod' library is appropriate for the validation logic used in this file.
3-11: LGTM: Well-structured custom error classThe
ParameterValidationErrorclass is well-implemented:
- It correctly extends the built-in
Errorclass.- The constructor is properly defined with a message and an errors object.
- The class name is appropriately set.
This class provides a good mechanism for handling and reporting validation errors.
1-24: Overall: Well-implemented validation utilityThis new file,
validate.ts, provides a robust implementation for validating share link status parameters. It includes:
- A custom error class for detailed error reporting.
- A validation function using the zod library for schema validation.
The code is well-structured, follows TypeScript best practices, and provides clear error handling. The only suggested improvement is to add type information for the
parametersargument in thevalidateSHLinkStatusParameterfunction.Great job on implementing this validation utility!
src/usecases/shlinks/get-shlink.ts (3)
7-7: LGTM: Function signature updated correctly.The addition of the optional
statusparameter aligns with the PR objectives and maintains backward compatibility.
10-11: LGTM:logicOperatorinitialized correctly.The
logicOperatoris properly initialized as an empty object, setting the stage for dynamic query construction based on the status.
18-23: LGTM: Query construction and repository call implemented correctly.The query construction using
logicOperatorand the subsequent repository call are implemented correctly. The use of the spread operator to mergelogicOperatorinto the query object is appropriate and maintains good readability.src/app/utils/error-handler.ts (1)
9-9: LGTM: New import statement for ParameterValidationErrorThe import statement for
ParameterValidationErroris correctly placed and necessary for the new error handling logic.src/app/api/v1/share-links/route.ts (5)
6-6: LGTM: Importing validation function for status parameterThe import of
validateSHLinkStatusParameteris a good practice for input validation, enhancing the robustness of the API.
67-74: LGTM: Well-documented new query parameterThe added Swagger documentation for the
statusquery parameter is clear and comprehensive. It accurately describes the parameter's purpose, possible values, and location in the request.
92-97: LGTM: Proper validation and use of status parameterThe addition of
validateSHLinkStatusParameterensures that thestatusparameter is validated before use. The inclusion of thestatusparameter in thegetSHLinkUseCasecall correctly implements the new filtering functionality.
19-19: Remove redundantdynamicexportWhen using
Cache-Controlheaders to control caching, theexport const dynamic = 'force-dynamic';line becomes redundant.Apply this diff to remove the redundant export:
-export const dynamic = 'force-dynamic';Likely invalid or redundant comment.
50-52: Avoid logging sensitive user informationLogging user identifiers such as
dto.userIdanddto.namecan expose personally identifiable information (PII) and pose security risks. Consider omitting or masking sensitive data in logs.Apply this diff to remove PII from the log statement:
logger.info( - `Creating a share link API with parameters, ${JSON.stringify({ name: dto.name, userId: dto.userId })}`, + 'Creating a new share link.', );Likely invalid or redundant comment.
src/app/utils/validate.ts
Outdated
| export const validateSHLinkStatusParameter = (parameters) => { | ||
| const statusSchema = z.enum(['expired', 'active', 'inactive']); | ||
|
|
||
| const result = statusSchema.safeParse(parameters.status); | ||
|
|
||
| if (!result.success) { | ||
| throw new ParameterValidationError('Invalid status parameter', { | ||
| status: result.error.errors.map((err) => err.message), | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
LGTM with a minor suggestion: Consider typing the 'parameters' argument
The validateSHLinkStatusParameter function is well-implemented:
- It correctly uses zod to create and validate against a schema for the status parameter.
- The error handling is appropriate, throwing a
ParameterValidationErrorwith detailed information.
Consider adding a type for the parameters argument to improve type safety. You could define an interface like this:
interface SHLinkParameters {
status?: string;
}Then update the function signature:
export const validateSHLinkStatusParameter = (parameters: SHLinkParameters) => {
// ... rest of the function
};This would provide better type checking and documentation for the expected input.
| status?: string; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
inconsistent spacing?
src/mappers/shlink-mapper.test.ts
Outdated
| @@ -172,6 +172,7 @@ describe('mapModelToMiniDto', () => { | |||
| passwordRequired: true, | |||
| url: 'http://localhost:3000/viewer#shlink:/eyJsYWJlbCI6Im5hbWUiLCJ1cmwiOiJodHRwOi8vbG9jYWxob3N0OjMwMDAvYXBpL3YxL3NoYXJlLWxpbmtzL2xpbmstaWQiLCJmbGFnIjoiUCJ9', | |||
| name: 'name', | |||
| active:true, | |||
There was a problem hiding this comment.
make spacing consistent
src/mappers/shlink-mapper.test.ts
Outdated
| @@ -258,6 +260,7 @@ describe('mapModelToMiniDto', () => { | |||
| name: 'name', | |||
| url: 'http://localhost:3000/viewer#shlink:/eyJsYWJlbCI6Im5hbWUiLCJ1cmwiOiJodHRwOi8vbG9jYWxob3N0OjMwMDAvYXBpL3YxL3NoYXJlLWxpbmtzL2xpbmstaWQiLCJmbGFnIjoiUCJ9', | |||
| expiryDate: date, | |||
| active:true, | |||
There was a problem hiding this comment.
make spacing consistent
Implemented share link filters:
Expired - if expiry date defined and less than today
Active - Active if not expired and active = true
Inactive - If active = false
Summary by CodeRabbit
New Features
Bug Fixes
Documentation