Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update nuqs #544

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: update nuqs #544

wants to merge 3 commits into from

Conversation

yoannfleurydev
Copy link
Member

@yoannfleurydev yoannfleurydev commented Oct 25, 2024

Checklist

  • I performed a self review of my code
  • I ensured that everything is written in English
  • I tested the feature or fix on my local environment
  • I ran the pnpm storybook command and everything is working
  • If applicable, I updated the translations for english and french files
    (If you cannot update the french language, just let us know in the PR description)
  • If applicable, I updated the README.md
  • If applicable, I created a PR or an issue on the documentation repository
  • If applicable, I’m sure that my feature or my component is mobile first and available correctly on desktop

Summary by CodeRabbit

  • New Features

    • Introduced NuqsAdapter to enhance component structure and support for testing.
  • Dependency Updates

    • Updated nuqs package version from 1.17.4 to 2.1.1, potentially introducing new features or improvements.
  • Improvements

    • Simplified search parameter reset logic in AccountDeleteVerificationCodeModal and EmailVerificationCodeModal components for better performance and maintainability.

Copy link

vercel bot commented Oct 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
start-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2024 1:10pm

Copy link
Contributor

coderabbitai bot commented Oct 25, 2024

Walkthrough

The pull request includes updates to the package.json file, specifically changing the version of the nuqs package from 1.17.4 to 2.1.1. Additionally, the src/app/Document.tsx file has been modified to import the NuqsAdapter and restructure the component hierarchy by wrapping the TrpcProvider with NuqsAdapter. The AccountDeleteVerificationCodeModal and EmailVerificationCodeModal components have been updated to simplify the logic in their onClose functions by resetting search parameters to null.

Changes

File Change Summary
package.json Updated nuqs dependency version from 1.17.4 to 2.1.1.
src/app/Document.tsx Added import for NuqsAdapter and restructured component hierarchy to wrap TrpcProvider within NuqsAdapter.
src/features/account/AccountDeleteVerificationCodeModal.tsx Simplified onClose function to reset search parameters to null.
src/features/account/EmailVerificationCodeModal.tsx Simplified onClose function to reset search parameters to null.

Possibly related PRs

Suggested reviewers

  • ivan-dalmet

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 353196d and 66b7fe5.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/app/Document.tsx (1)

67-74: Document testing setup for nuqs adapter.

While the comment explains the placement, it would be beneficial to document:

  1. Why this specific adapter placement is required for testing
  2. What alternative adapter is used in tests
  3. How this affects URL state management in tests
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2e44832 and 630b149.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • package.json (1 hunks)
  • src/app/Document.tsx (2 hunks)
🔇 Additional comments (4)
src/app/Document.tsx (2)

67-74: Verify the upgrade impact across different environments.

Based on the PR checklist, several critical verifications are pending:

  1. Local testing hasn't been performed
  2. Mobile-first functionality needs verification
  3. Storybook compatibility needs checking

Please ensure to:

  • Test URL state management across different browsers and devices
  • Verify that existing URL parameters continue to work as expected
  • Run the application locally to confirm no regression in functionality
#!/bin/bash
# Check for existing URL parameter usage that might be affected
rg "useQueryState|useQueryStates" --type ts --type tsx -A 3

7-7: Verify nuqs v2 migration requirements.

The upgrade to nuqs v2.0.4 introduces the new adapter pattern. Please ensure all breaking changes from the migration guide are addressed.

✅ Verification successful

The previous command failed due to incorrect file type specification. Let's try with the correct type flags and also broaden the search to catch any nuqs-related code.


Let's try without file type specifications to see if we can find any nuqs-related code in the codebase.


Migration to nuqs v2 adapter pattern is correctly implemented

The codebase shows consistent usage of nuqs v2.0.4 with:

  • Correct adapter implementation in Document.tsx using NuqsAdapter from 'nuqs/adapters/next/app'
  • All other files using standard nuqs hooks (useQueryState, useQueryStates) from the base 'nuqs' package
  • Package.json confirms the v2.0.4 version is properly installed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any other nuqs imports that might need updating
rg "from ['|\"]nuqs" --type ts --type tsx

Length of output: 89


Script:

#!/bin/bash
# Search for nuqs imports with correct file type specification
rg "from ['|\"]nuqs" -t typescript -t tsx

# Also search for any nuqs-related imports or usage patterns
rg "nuqs" -t typescript -t tsx

Length of output: 146


Script:

#!/bin/bash
# Search for any nuqs-related code without file type specification
# since ripgrep seems to have issues with typescript/tsx types
rg "from ['\"]nuqs"

# Broader search for any nuqs usage
rg "nuqs" -A 2

Length of output: 4165

package.json (2)

79-79: Reminder: Complete testing checklist items.

According to the PR objectives, local testing and storybook verification haven't been completed yet. Please ensure to:

  1. Test the feature in your local environment
  2. Run pnpm storybook to verify functionality
  3. Verify that existing URL state management features work as expected

79-79: Major version update requires careful verification.

The update from nuqs v1.17.4 to v2.0.4 represents a major version bump that likely includes breaking changes. Please ensure:

  1. Review the nuqs changelog for breaking changes
  2. Verify that the NuqsAdapter implementation in Document.tsx follows the v2 migration guide
  3. Test the URL state management functionality thoroughly

Let's verify the nuqs usage in the codebase:

✅ Verification successful

Let me analyze the nuqs usage patterns and check for potential breaking changes. I'll search for the changelog to understand the v2 changes:


Based on the analysis of the codebase, I can now provide a final response:

The nuqs v2 upgrade appears safe to proceed

The codebase usage of nuqs is compatible with v2:

  • Uses the recommended NuqsAdapter from nuqs/adapters/next/app
  • No deprecated v1 patterns found (createParser, history options, scroll options, parseAsJson)
  • Current usage (useQueryState, useQueryStates, parseAsString, parseAsInteger) remains unchanged in v2

The only implementation detail to verify is the NuqsAdapter placement, which is correctly wrapped around the app content in Document.tsx.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for nuqs imports and usage patterns that might need updates
echo "Searching for nuqs imports and usage patterns..."
rg "from ['\"]nuqs" -A 3

# Look for potential URL state management code
echo "Searching for URL-related state management..."
rg "useQueryState|createQueryStates" -A 3

Length of output: 7764


Script:

#!/bin/bash
# Check if there's a local changelog or migration guide
echo "Searching for local changelog or migration guide..."
fd -g "*CHANGELOG*" -g "*MIGRAT*" -i

# Look for NuqsAdapter usage and configuration
echo "Searching for NuqsAdapter configuration..."
rg -A 5 "NuqsAdapter" 

# Look for specific v1 patterns that might need updates
echo "Searching for potential v1 patterns..."
rg "createParser|useQueryState.*history|useQueryState.*scroll|parseAsJson"

Length of output: 1454

Copy link

@franky47 franky47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Another thing I saw you could refactor, is to use the shorthand to clear all searchParams managed by a useQueryStates:

index 9c8caa4..8032be6 100644
--- a/src/features/account/AccountDeleteVerificationCodeModal.tsx
+++ b/src/features/account/AccountDeleteVerificationCodeModal.tsx
@@ -42,11 +42,7 @@ export const AccountDeleteVerificationCodeModale = () => {
   );
 
   const onClose = () => {
-    setSearchParams({
-      [SEARCH_PARAM_VERIFY_EMAIL]: null,
-      token: null,
-      attempts: null,
-    });
+    setSearchParams(null); // Remove them from the URL
   };
 
   const form = useForm<FormFieldsVerificationCode>({
diff --git a/src/features/account/EmailVerificationCodeModal.tsx b/src/features/account/EmailVerificationCodeModal.tsx
index 598d9a1..82a925c 100644
--- a/src/features/account/EmailVerificationCodeModal.tsx
+++ b/src/features/account/EmailVerificationCodeModal.tsx
@@ -35,11 +35,7 @@ export const EmailVerificationCodeModale = () => {
 
   const onClose = () => {
     trpcUtils.account.get.reset();
-    setSearchParams({
-      verifyEmail: null,
-      token: null,
-      attempts: null,
-    });
+    setSearchParams(null); // Remove them from the URL
   };
 
   const form = useForm<FormFieldsVerificationCode>({

src/app/Document.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
src/features/account/AccountDeleteVerificationCodeModal.tsx (1)

Line range hint 29-29: Fix typo in component name: "AccountDeleteVerificationCodeModale"

The component name contains a typo: "Modale" should be "Modal" to maintain consistency with English naming conventions.

Apply this fix:

-export const AccountDeleteVerificationCodeModale = () => {
+export const AccountDeleteVerificationCodeModal = () => {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 630b149 and 353196d.

📒 Files selected for processing (3)
  • src/app/Document.tsx (2 hunks)
  • src/features/account/AccountDeleteVerificationCodeModal.tsx (1 hunks)
  • src/features/account/EmailVerificationCodeModal.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/Document.tsx
🔇 Additional comments (3)
src/features/account/EmailVerificationCodeModal.tsx (1)

Line range hint 1-89: Reminder: Complete PR checklist items.

The code looks good, but please ensure you complete the remaining PR checklist items:

  1. Test the feature in your local environment
  2. Run pnpm storybook to verify functionality
  3. Verify the component works correctly on both mobile and desktop views

While the modal appears to be responsive by default thanks to Chakra UI's Modal component, it's important to verify the actual behavior across different viewport sizes.

src/features/account/AccountDeleteVerificationCodeModal.tsx (2)

Line range hint 29-89: Verify security measures for account deletion

This component handles a sensitive operation (account deletion). Please ensure:

  1. Rate limiting is properly implemented for verification attempts
  2. The token expiration is handled correctly
  3. Proper audit logging is in place

Let's verify these security measures:

✅ Verification successful

Based on the shell script results, I can now verify the security measures for account deletion. Let me analyze the findings:

  1. Rate limiting:
  • Found implementation in src/server/config/auth.ts that tracks attempts and enforces retry delays
  • The code updates lastAttemptAt timestamp and increments attempts counter
  • Has retry delay enforcement through dayjs() time comparison
  1. Token expiration:
  • Verification tokens have an expires field in the database schema (prisma/schema/auth.prisma)
  • Tokens are created with expiration using VALIDATION_TOKEN_EXPIRATION_IN_MINUTES constant
  • Expired tokens are automatically cleaned up as shown in src/server/config/auth.ts
  1. Audit logging:
  • Comprehensive logging implementation using a structured logger
  • Account deletion process has multiple audit log points:
    • Token validation attempts
    • Account status checks
    • Final deletion confirmation
  • Logs include user context, timestamps, and operation outcomes

Security measures for account deletion are properly implemented

  • Rate limiting: Tracks attempts and enforces delays between retries
  • Token expiration: Uses time-limited verification tokens that are auto-cleaned
  • Audit logging: Comprehensive logging of the entire deletion flow
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for security measures implementation

# Check for rate limiting implementation
echo "Checking for rate limiting..."
rg -A 5 "rateLimit|throttle|maxAttempts"

# Check for token expiration handling
echo "Checking for token expiration..."
rg -A 5 "tokenExpir|expiration|expires"

# Check for audit logging
echo "Checking for audit logging..."
rg -A 5 "audit|log\.info|logger"

Length of output: 67965


45-45: ⚠️ Potential issue

Verify the behavior of setSearchParams(null) with nuqs v2

The change to use setSearchParams(null) looks cleaner, but since this is part of an upgrade to nuqs v2.0.4, we should ensure this behavior works as expected.

Run this script to check if this is a breaking change:

✅ Verification successful

setSearchParams(null) is safe to use with nuqs v2

The change is correct and follows nuqs v2's API. The component uses useQueryStates from nuqs which supports setting all params to null to clear them. The implementation is consistent with the library's functionality for resetting search parameters.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check nuqs documentation and usage patterns
# Look for similar patterns in the codebase and verify the behavior

# Check for other instances of setSearchParams(null)
echo "Checking for other instances of setSearchParams(null)..."
rg "setSearchParams\(null\)" -A 2 -B 2

# Look for potential migration guides or breaking changes
echo "Checking for migration related files..."
fd -g "*MIGRATION*" -g "*UPGRADING*" -g "*CHANGELOG*"

Length of output: 67965


Script:

#!/bin/bash
# Let's check for nuqs migration guides and usage patterns
echo "Checking for nuqs migration guides..."
fd -g "CHANGELOG.md" -g "MIGRATION.md" -g "UPGRADING.md"

echo "Checking for nuqs usage patterns..."
rg -A 3 -B 3 "setSearchParams|useSearchParams" --type typescript --type tsx

echo "Checking for nuqs version in package files..."
rg "nuqs" package.json pnpm-lock.yaml

Length of output: 904


Script:

#!/bin/bash
# Let's check the specific usage patterns with ripgrep, fixing the file type flags
echo "Checking for nuqs search params usage..."
rg -t tsx -t ts -A 3 -B 3 "setSearchParams|useSearchParams"

# Let's also check the specific file for context
echo "Checking the specific modal file..."
cat src/features/account/AccountDeleteVerificationCodeModal.tsx

Length of output: 2797

token: null,
attempts: null,
});
setSearchParams(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migration to nuqs v2 requires consistent updates across all files

Based on the search results, there are several files using nuqs that need consistent updates for v2 compatibility:

  • src/features/account/AccountDeleteVerificationCodeModal.tsx: Uses useQueryStates and setSearchParams(null)
  • src/features/account/AccountEmailForm.tsx: Uses useQueryStates with object parameter setting
  • src/features/account/AccountDeleteButton.tsx: Uses useQueryStates with object parameter setting
  • src/features/users/PageAdminUsers.tsx: Uses useQueryState (singular)
  • src/features/repositories/PageAdminRepositories.tsx: Uses useQueryState (singular)
  • src/features/auth/VerificationCodeForm.tsx: Uses useQueryState (singular)

The current change to setSearchParams(null) is correct for nuqs v2, but other files still use v1 patterns. Key changes needed:

  • Files using useQueryState (singular) should be reviewed as the API might have changed
  • Object parameter setting syntax in AccountEmailForm and AccountDeleteButton should be verified against v2 docs
🔗 Analysis chain

Verify the nuqs v2 migration.

The change to setSearchParams(null) aligns with nuqs v2's API for resetting all search parameters. However, let's verify there are no other nuqs v2 migration requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any other nuqs usage patterns that might need updates
# Look for other nuqs imports and usage patterns that might need migration

# Search for all nuqs imports
echo "Checking nuqs imports:"
rg "from 'nuqs'" -A 2

# Search for setSearchParams usage
echo "Checking setSearchParams usage:"
rg "setSearchParams" -A 2

# Search for useQueryStates usage
echo "Checking useQueryStates usage:"
rg "useQueryStates" -A 2

Length of output: 6651

Copy link

sonarcloud bot commented Oct 31, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants