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

Add loading indicators in innovation library #7198

Conversation

reactoholic
Copy link
Contributor

@reactoholic reactoholic commented Nov 14, 2024

  • Add loading indicators in innovation library
  • Fetch packs on every page visit in order to have the UI up to date (previous behaviour was to display only the cached packs)

Summary by CodeRabbit

  • New Features

    • Added loading state management to various components, enhancing user experience during data fetching.
    • Introduced Skeleton components for visual feedback while loading data in InnovationPacksView and LibraryTemplatesView.
  • Bug Fixes

    • Improved data fetching logic to ensure fresh data is retrieved from the network.
  • Documentation

    • Updated component interfaces to include optional loading properties for better state handling.

Signed-off-by: reactoholic <petar.georgiev.kolev@gmail.com>
@reactoholic reactoholic added client enhancement UX Enhanced user experience labels Nov 14, 2024
@reactoholic reactoholic requested a review from ccanos November 14, 2024 12:02
Copy link

coderabbitai bot commented Nov 14, 2024

Walkthrough

The pull request introduces several modifications across multiple components, primarily focusing on the addition of a loading state to improve user experience during data fetching. Components such as DashboardInnovationPacks, InnovationPacksView, DashboardLibraryTemplates, and LibraryTemplatesView have been updated to include a new optional loading property, allowing them to render loading indicators. Additionally, the InnovationLibraryPage component has been modified to ensure fresh data fetching and to pass the loading state down to child components. The ContributeCardSkeleton component has also been updated by removing the explicit import of React.

Changes

File Change Summary
src/core/ui/card/ContributeCardSkeleton.tsx Removed import of React.
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx Updated DashboardInnovationPacksProps to include loading property; moved innovationPackValueGetter function to the end.
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx Extended InnovationPacksViewProps with loading; added conditional rendering for loading state.
src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx Changed DashboardLibraryTemplatesProps from interface to type alias; added loading property.
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx Updated LibraryTemplatesViewProps to include loading; modified rendering logic to handle loading state.
src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx Updated query to include loading state; passed innovationLibraryLoading to child components.

Possibly related issues

Possibly related PRs

Suggested labels

translations

Suggested reviewers

  • reactoholic
  • bobbykolev

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.

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

@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 (6)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (2)

17-22: Consider adding error handling for data fetching scenarios.

While the loading state is now handled, the component could benefit from error handling to manage failed data fetching scenarios.

Consider adding error boundaries or implementing error state handling:

 type DashboardInnovationPacksProps = {
   headerTitle: ReactNode;
   dialogTitle: ReactNode;
   innovationPacks: (Identifiable & InnovationPackCardProps)[] | undefined;
   loading?: boolean;
+  error?: Error;
 };

 const DashboardInnovationPacks = ({
   loading,
   headerTitle,
   dialogTitle,
   innovationPacks,
+  error,
 }: DashboardInnovationPacksProps) => {

63-68: Consider memoizing the value getter function.

Since innovationPackValueGetter is used in filtering operations, memoizing it could improve performance for large datasets.

Consider using useCallback:

+const innovationPackValueGetter = useCallback(
-function innovationPackValueGetter
 (innovationPack: Identifiable & InnovationPackCardProps): ValueType {
   return {
     id: innovationPack.id,
     values: [innovationPack.displayName, ...(innovationPack.tags ?? [])],
   };
-}
+}, []);
src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (1)

49-49: LGTM! Consider adding error states

The loading state implementation is clean and consistent across both components. Consider also passing error states to these components for a complete user experience.

Example implementation:

const { 
  data: innovationLibraryData, 
  loading: innovationLibraryLoading,
+ error: innovationLibraryError 
} = useInnovationLibraryQuery({
  fetchPolicy: 'network-only',
});

<DashboardInnovationPacks
  // ... existing props
  loading={innovationLibraryLoading}
+ error={innovationLibraryError}
/>

<DashboardLibraryTemplates
  // ... existing props
  loading={innovationLibraryLoading}
+ error={innovationLibraryError}
/>

Also applies to: 56-56

src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (2)

13-13: LGTM! Consider adding JSDoc comments.

The interface extension and new import look good. Consider adding JSDoc comments to document the loading prop's purpose and behavior.

 interface InnovationPacksViewProps extends PageContentBlockProps {
   filter: string[];
   headerTitle: ReactNode;
   innovationPacks: (Identifiable & InnovationPackCardProps)[] | undefined;
   expanded?: boolean;
   onFilterChange: Dispatch<string[]>;
   onDialogOpen?: () => void;
   onDialogClose?: () => void;
   hasMore?: boolean;
+  /** Indicates whether the innovation packs are currently being loaded */
   loading?: boolean;
 }

Also applies to: 15-24


53-64: Consider accessibility and maintainability improvements.

While the loading state implementation works, there are a few suggestions for improvement:

  1. Extract magic numbers (5, 240, 300) into named constants
  2. Add aria-busy attribute for accessibility
  3. Consider using a loading message for screen readers
+ const SKELETON_COUNT = 5;
+ const SKELETON_WIDTH = 240;
+ const SKELETON_HEIGHT = 300;

  <ScrollableCardsLayoutContainer minHeight={0} orientation={expanded ? 'vertical' : undefined} sameHeight>
+   <div aria-busy={loading} aria-live="polite">
      {loading
-       ? Array.from({ length: 5 }).map((_, idx) => (
+       ? Array.from({ length: SKELETON_COUNT }).map((_, idx) => (
            <Skeleton
              key={idx}
-             width={240}
-             height={300}
+             width={SKELETON_WIDTH}
+             height={SKELETON_HEIGHT}
              animation="pulse"
              variant="rectangular"
              sx={{ borderRadius: 1 }}
+             aria-hidden="true"
            />
          ))
        : innovationPacks?.map(({ id, ...cardProps }) => <InnovationPackCard key={id} {...cardProps} />)}
+     {loading && <span className="sr-only">{t('common.loading')}</span>}
+   </div>
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

91-111: Consider extracting magic numbers and improving accessibility

While the loading state implementation is good, there are a few improvements that could be made:

+ const SKELETON_COUNT = 5;
+ const SKELETON_WIDTH = 248;
+ const SKELETON_HEIGHT = 262;

  <ScrollableCardsLayoutContainer minHeight={0} orientation={expanded ? 'vertical' : undefined} sameHeight>
-   {loading
-     ? Array.from({ length: 5 }).map((_, idx) => (
+   {loading
+     ? Array.from({ length: SKELETON_COUNT }).map((_, idx) => (
          <Skeleton
            key={idx}
-           width={248}
-           height={262}
+           width={SKELETON_WIDTH}
+           height={SKELETON_HEIGHT}
            animation="pulse"
            variant="rectangular"
-           sx={{ borderRadius: 1 }}
+           sx={{ borderRadius: 1 }}
+           aria-label="Loading template card"
          />
        ))
-     : templates?.map(({ template, innovationPack }) => (
+     : templates?.length ? templates.map(({ template, innovationPack }) => (
          <TemplateCard
            key={template.id}
            template={template}
            innovationPack={innovationPack}
            onClick={() => onClick(template)}
          />
-       ))}
+       )) : (
+         <Box sx={{ textAlign: 'center', py: 2 }}>No templates available</Box>
+       )}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c34a16b and 39dac64.

📒 Files selected for processing (6)
  • src/core/ui/card/ContributeCardSkeleton.tsx (0 hunks)
  • src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (3 hunks)
  • src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (5 hunks)
  • src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx (3 hunks)
  • src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (5 hunks)
  • src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (2 hunks)
💤 Files with no reviewable changes (1)
  • src/core/ui/card/ContributeCardSkeleton.tsx
🧰 Additional context used
📓 Path-based instructions (5)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
🔇 Additional comments (12)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (2)

8-13: LGTM! Type definition changes look good.

The conversion to type alias and addition of the optional loading prop are well-structured and follow TypeScript best practices.


42-42: LGTM! Loading state properly propagated.

The loading prop is consistently passed to both the main view and dialog view, ensuring uniform loading state representation across the UI.

Also applies to: 54-54

src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (1)

17-19: Verify network failure handling

The network-only fetch policy ensures fresh data on every visit, which aligns with the requirements. However, we should ensure proper error handling for network failures.

Let's verify the error handling implementation:

src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx (3)

9-14: LGTM! Clean type definition with proper optional loading state.

The conversion to a type alias and addition of the optional loading prop is well-structured and follows TypeScript best practices.


28-33: LGTM! Props destructuring is clean and properly typed.

The component parameters are well-organized and maintain type safety.


Line range hint 28-89: Verify error handling during loading state.

The component should handle potential undefined states gracefully when templates are loading.

Let's verify the error handling in related components:

✅ Verification successful

Let me check the error handling implementation in the LibraryTemplatesView component.


Loading state is properly handled

The component implements robust error handling during loading states:

  • Uses optional chaining (templates?.map) to safely handle undefined templates
  • Displays loading skeletons when loading is true (5 placeholder cards with proper dimensions)
  • Gracefully handles the transition between loading and loaded states
  • Properly propagates loading state to child components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check error handling in related components

# Check for error handling patterns in LibraryTemplatesView
ast-grep --pattern 'if ($loading) {
  $$$
}'

# Check for null checks on templates prop
ast-grep --pattern 'templates?.map'

# Look for error boundaries or error states
rg -l 'Error|error' src/domain/InnovationPack/

Length of output: 765


Script:

#!/bin/bash
# Check LibraryTemplatesView implementation for loading states
cat src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx

# Check if there are any loading skeletons or placeholders
rg -A 5 "loading" src/domain/InnovationPack/DashboardLibraryTemplates/

Length of output: 8709

src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (2)

36-38: LGTM! Props destructuring is clean and type-safe.

The addition of the loading prop and spread operator for remaining props is well implemented.


53-64: Verify loading state behavior across the application.

Let's verify that the loading state is properly managed and consistent with other components.

✅ Verification successful

Loading state implementation is consistent across the Innovation Pack domain

The loading state implementation in InnovationPacksView.tsx follows the established patterns within the Innovation Pack domain:

  • The loading prop is consistently typed as optional boolean across all components
  • Skeleton components are used consistently for loading states in both list and card views
  • Loading states are properly propagated from data fetching operations to UI components
  • The implementation aligns with other views like LibraryTemplatesView and InnovationPackCardHorizontal
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check loading state implementation in related components

# Check for loading prop usage in related components
echo "Checking loading prop usage in related components:"
rg -t typescript -t tsx "loading\?:\s*boolean" src/domain/InnovationPack

# Check for Skeleton usage patterns
echo "\nChecking Skeleton implementation patterns:"
rg -t typescript -t tsx "Skeleton.*animation.*pulse" src/domain/InnovationPack

# Check for accessibility patterns
echo "\nChecking accessibility patterns:"
rg -t typescript -t tsx "aria-(busy|live)" src/domain/InnovationPack

Length of output: 620


Script:

#!/bin/bash
# Let's try without file type filters since typescript/tsx extensions weren't recognized
echo "Checking loading prop usage in related components:"
rg "loading\?:\s*boolean" src/domain/InnovationPack/

echo -e "\nChecking Skeleton component usage patterns:"
rg "Skeleton.*animation.*pulse" src/domain/InnovationPack/

echo -e "\nChecking other loading state implementations:"
rg "loading\s*[=:]" src/domain/InnovationPack/

echo -e "\nChecking accessibility patterns:"
rg "aria-(busy|live)" src/domain/InnovationPack/

# Let's also check for other Skeleton implementations
echo -e "\nChecking all Skeleton usages:"
rg "<Skeleton" src/domain/InnovationPack/

Length of output: 4934

src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (4)

1-2: LGTM! Type definitions are well-structured

The addition of the optional loading prop maintains backward compatibility while enabling loading state functionality.

Also applies to: 22-33


Line range hint 34-89: LGTM! Props handling follows React best practices

The loading prop is correctly destructured and integrated into the existing props structure.


112-129: LGTM! Show more functionality remains consistent

The implementation maintains the existing responsive behavior and user experience.


32-32: Verify loading state propagation from parent components

Let's ensure the loading state is properly handled in parent components.

✅ Verification successful

Loading state is properly implemented in the component

The loading prop is correctly used within the LibraryTemplatesView component to display skeleton UI elements while content is loading. When loading is true, it renders 5 skeleton cards instead of the actual template cards, providing a good user experience during data fetching.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components that render LibraryTemplatesView
rg -A 5 "LibraryTemplatesView" --type ts --type tsx

# Search for loading prop usage in parent components
ast-grep --pattern 'loading={$_}'

Length of output: 115


Script:

#!/bin/bash
# Search for components that render LibraryTemplatesView with correct file extensions
rg -A 5 "LibraryTemplatesView" -t ts -t tsx

# Search for files containing LibraryTemplatesView in their name
fd LibraryTemplatesView

# Search for loading prop usage in parent components
rg -B 3 -A 3 "<LibraryTemplatesView.*loading" -t ts -t tsx

Length of output: 263


Script:

#!/bin/bash
# Search for components that render LibraryTemplatesView
rg "LibraryTemplatesView" --type=typescript --type=typescriptreact

# Look for the content of the file to understand the loading prop usage
cat src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx

# Search for any imports of LibraryTemplatesView
rg "import.*LibraryTemplatesView" --type=typescript --type=typescriptreact

Length of output: 4950

@reactoholic reactoholic force-pushed the client-web/7174-loading-indicator-missing-for-innovation-library branch from 39dac64 to 08ac36d Compare November 14, 2024 12:12
Signed-off-by: reactoholic <petar.georgiev.kolev@gmail.com>
Copy link

@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/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

53-64: Consider these improvements to the loading state implementation.

While the loading state implementation works, there are several potential improvements:

  1. The number of skeleton placeholders should match the expected number of items
  2. Skeleton dimensions should match InnovationPackCard dimensions for consistency
  3. Add accessibility attributes for screen readers
  4. Consider performance optimization

Here's a suggested implementation:

+ const CARDS_PER_ROW = 5;
+ const skeletons = useMemo(() => (
+   Array.from({ length: CARDS_PER_ROW }).map((_, idx) => (
+     <Skeleton
+       key={idx}
+       width={240}
+       height={300}
+       animation="pulse"
+       variant="rectangular"
+       sx={{ borderRadius: 1 }}
+       aria-label={t('common.loading-innovation-pack')}
+     />
+   ))
+ ), [t]);

  <ScrollableCardsLayoutContainer minHeight={0} orientation={expanded ? 'vertical' : undefined} sameHeight>
-   {loading
-     ? Array.from({ length: 5 }).map((_, idx) => (
-         <Skeleton
-           key={idx}
-           width={240}
-           height={300}
-           animation="pulse"
-           variant="rectangular"
-           sx={{ borderRadius: 1 }}
-         />
-       ))
-     : innovationPacks?.map(({ id, ...cardProps }) => <InnovationPackCard key={id} {...cardProps} />)}
+   {loading ? skeletons : innovationPacks?.map(({ id, ...cardProps }) => <InnovationPackCard key={id} {...cardProps} />)}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 39dac64 and 08ac36d.

📒 Files selected for processing (4)
  • src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (3 hunks)
  • src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (5 hunks)
  • src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx (4 hunks)
  • src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx
  • src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx
🧰 Additional context used
📓 Path-based instructions (2)
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
🔇 Additional comments (4)
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (2)

Line range hint 1-24: LGTM! Clean interface extension with proper typing.

The interface changes and imports are well-structured. The optional loading prop is correctly typed, and the interface properly extends PageContentBlockProps.


Line range hint 26-38: LGTM! Props handling follows React best practices.

The component correctly destructures the loading prop and properly handles the spread operator for remaining props.

src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (2)

Line range hint 22-33: LGTM! Type definitions are well-structured

The addition of the optional loading prop to LibraryTemplatesViewProps is properly typed and follows TypeScript best practices.


Line range hint 34-46: LGTM! Component implementation is clean

The loading prop is properly destructured and integrated into the component following React best practices.

Copy link
Contributor

@ccanos ccanos left a comment

Choose a reason for hiding this comment

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

I think it doesn't look good on mobile, can you check it?

Copy link

@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 (3)
src/domain/InnovationPack/CreateInnovationPackDialog/CreateInnovationPackDialog.tsx (1)

Line range hint 26-42: Consider enhancing error handling and user feedback in mutation handler.

The current implementation could benefit from better error handling and user feedback:

  1. No error handling for failed mutations
  2. No success notification when pack is created
  3. Silent failure when accountId is undefined

Consider applying this improvement:

 const handleSubmit = async (formData: InnovationPackFormValues) => {
   if (!accountId) {
+    console.error('Cannot create innovation pack: accountId is undefined');
     return;
   }
-  const { data } = await createInnovationPack({
-    variables: {
-      packData: {
-        accountID: accountId,
-        nameID: formData.nameID,
-        profileData: {
-          displayName: formData.profile.displayName,
-          description: formData.profile.description,
-        },
-      },
-    },
-    refetchQueries: ['AdminInnovationPacksList', 'AccountInformation', 'InnovationLibrary'],
-  });
-  if (data?.createInnovationPack.nameID) {
-    setIsOpen(false);
-  }
+  try {
+    const { data } = await createInnovationPack({
+      variables: {
+        packData: {
+          accountID: accountId,
+          nameID: formData.nameID,
+          profileData: {
+            displayName: formData.profile.displayName,
+            description: formData.profile.description,
+          },
+        },
+      },
+      refetchQueries: ['AdminInnovationPacksList', 'AccountInformation', 'InnovationLibrary'],
+    });
+    if (data?.createInnovationPack.nameID) {
+      setIsOpen(false);
+      // Assuming you have a notification system
+      showNotification({
+        message: t('pages.admin.innovation-packs.createSuccess'),
+        severity: 'success',
+      });
+    }
+  } catch (error) {
+    console.error('Failed to create innovation pack:', error);
+    showNotification({
+      message: t('pages.admin.innovation-packs.createError'),
+      severity: 'error',
+    });
+  }
 };
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

Line range hint 1-26: LGTM! Consider making the loading prop required.

The interface extension and imports look good. However, since the component now heavily relies on the loading state for conditional rendering, consider making the loading prop required instead of optional to ensure proper handling at all times.

interface InnovationPacksViewProps extends PageContentBlockProps {
  filter: string[];
  headerTitle: ReactNode;
  innovationPacks: (Identifiable & InnovationPackCardProps)[] | undefined;
  expanded?: boolean;
  onFilterChange: Dispatch<string[]>;
  onDialogOpen?: () => void;
  onDialogClose?: () => void;
  hasMore?: boolean;
-  loading?: boolean;
+  loading: boolean;
}
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

95-105: Add accessibility attributes to loading skeletons

The loading skeletons should include ARIA attributes to improve accessibility for screen readers.

 <Skeleton
   key={idx}
   width={gutters(12)(theme)}
   height={gutters(13)(theme)}
   animation="pulse"
   variant="rectangular"
+  aria-label="Loading template card"
+  role="progressbar"
   sx={{ borderRadius: 1 }}
 />
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 08ac36d and b03396e.

📒 Files selected for processing (4)
  • src/domain/InnovationPack/CreateInnovationPackDialog/CreateInnovationPackDialog.tsx (1 hunks)
  • src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (5 hunks)
  • src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (5 hunks)
  • src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/domain/InnovationPack/CreateInnovationPackDialog/CreateInnovationPackDialog.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

Pattern src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:

  • SQL Injection
  • XSS (Cross-Site Scripting)
  • CSRF (Cross-Site Request Forgery)
  • Insecure dependencies
  • Sensitive data exposure
🔇 Additional comments (5)
src/domain/InnovationPack/CreateInnovationPackDialog/CreateInnovationPackDialog.tsx (1)

37-37: LGTM! Addition of 'InnovationLibrary' to refetchQueries.

The change ensures that the Innovation Library view is refreshed after creating a new pack, which aligns with the PR objective of keeping the UI up-to-date with fresh data.

src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (2)

Line range hint 28-40: LGTM! Props destructuring is clean and well-organized.

The component properly destructures all props including the new loading state, with appropriate default values where needed.


69-84: LGTM! Mobile-specific UI handling is well implemented.

The "Show All" button's conditional rendering properly considers the loading state and mobile view, with clean styling implementation.

src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (2)

Line range hint 24-35: LGTM: Type definitions are well-structured

The LibraryTemplatesViewProps type is well-defined with the new optional loading prop properly typed as boolean.


115-130: LGTM: Well-implemented conditional rendering

The "Show All" button implementation correctly handles both loading state and mobile view conditions, preventing premature button display during loading states.

Copy link
Contributor Author

@reactoholic reactoholic left a comment

Choose a reason for hiding this comment

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

Loogs very good!

@ccanos ccanos merged commit c2567b9 into develop Nov 15, 2024
3 checks passed
@ccanos ccanos deleted the client-web/7174-loading-indicator-missing-for-innovation-library branch November 15, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client enhancement UX Enhanced user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants