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: added dynamic pagination for environments (fixes #707) #784

Conversation

MohamedHamdanA
Copy link

@MohamedHamdanA MohamedHamdanA commented Feb 23, 2025

User description

Description

I have added dynamic pagination for the environment page

Fixes #707

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Added dynamic pagination to the environments page.

  • Introduced "Load More" button for additional data.

  • Implemented state management for pagination and loading.

  • Updated UI to handle empty and loading states effectively.


Changes walkthrough 📝

Relevant files
Enhancement
page.tsx
Implemented dynamic pagination and UI updates                       

apps/platform/src/app/(main)/(project)/[workspace]/[project]/@environment/page.tsx

  • Added pagination state management with useState.
  • Replaced full data fetch with paginated API calls.
  • Implemented "Load More" button for fetching additional environments.
  • Updated UI to handle empty states and loading indicators.
  • +60/-32 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    707 - PR Code Verified

    Compliant requirements:

    • Add dynamic pagination on environments page
    • Show first 10 environments by default
    • Add "Load More" button at end of page
    • Load More button should show next 10 environments on click

    Requires further human verification:

    • Verify that UI matches provided Figma design
    • Test pagination behavior with actual data to ensure smooth loading
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The API error handling is missing. Failed API requests should be handled gracefully with appropriate error messages to users.

    getEnvironments()
      .then(({ data, success }) => {
        if (success && data) {
          // On first page, replace; on subsequent pages, append new items
          setEnvironments(prev =>
            page === 0 ? data.items : [...prev, ...data.items]
          )
          // If the returned items are fewer than the limit, there's no more data
          if (data.items.length < limit) {
            setHasMore(false)
          }
        }
      })
      .finally(() => {
        setLoading(false)
      })
    Loading State

    Initial loading state is not handled properly. Users won't see any loading indicator when environments are first being fetched.

    {environments.length === 0 && !loading ? (
      // Display when no environments are present
      <div className="flex h-[95%] w-full flex-col items-center justify-center gap-y-8">

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error handling for failures

    Add error handling for the API call to handle failed environment fetches
    gracefully and inform users of any issues.

    apps/platform/src/app/(main)/(project)/[workspace]/[project]/@environment/page.tsx [50-66]

     getEnvironments()
    -  .then(({ data, success }) => {
    +  .then(({ data, success, error }) => {
         if (success && data) {
    -      // On first page, replace; on subsequent pages, append new items
           setEnvironments(prev =>
             page === 0 ? data.items : [...prev, ...data.items]
           )
    -      // If the returned items are fewer than the limit, there's no more data
           if (data.items.length < limit) {
             setHasMore(false)
           }
    +    } else {
    +      console.error('Failed to fetch environments:', error);
    +      setHasMore(false);
    +      // Show error state to user
    +      setError('Failed to load environments. Please try again.');
         }
    +  })
    +  .catch((err) => {
    +    console.error('Error fetching environments:', err);
    +    setHasMore(false);
    +    setError('Failed to load environments. Please try again.');
       })
       .finally(() => {
         setLoading(false)
       })
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds crucial error handling that was missing in the original code, including proper error state management and user feedback for failed API calls. This is important for production reliability and user experience.

    Medium
    Prevent memory leaks on unmount

    Add a cleanup function in useEffect to prevent state updates on unmounted
    component, which could lead to memory leaks.

    apps/platform/src/app/(main)/(project)/[workspace]/[project]/@environment/page.tsx [47-67]

     useEffect(() => {
    +  let isMounted = true;
       if (selectedProject) {
         setLoading(true)
         getEnvironments()
           .then(({ data, success }) => {
    +        if (!isMounted) return;
             if (success && data) {
               setEnvironments(prev =>
                 page === 0 ? data.items : [...prev, ...data.items]
               )
               if (data.items.length < limit) {
                 setHasMore(false)
               }
             }
           })
           .finally(() => {
    -        setLoading(false)
    +        if (isMounted) {
    +          setLoading(false)
    +        }
           })
       }
    +  return () => {
    +    isMounted = false;
    +  };
     }, [selectedProject, page, getEnvironments, setEnvironments])
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion prevents potential memory leaks by adding cleanup logic to handle component unmounting, which is important for React component lifecycle management and preventing state updates on unmounted components.

    Medium
    • More

    @rajdip-b
    Copy link
    Member

    Hey man, even though your PR is correct, we would need to take it down. Since another contributor has already dropped their PR for this same issue. Please refrain from taking up issues that are already assigned. That would save you your time.

    @rajdip-b rajdip-b closed this Feb 23, 2025
    @MohamedHamdanA
    Copy link
    Author

    Ok

    @MohamedHamdanA MohamedHamdanA deleted the issue-707-pagination branch February 23, 2025 10:28
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Add pagination on environments screen
    2 participants