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

Аdd skeleton loading indicators in the innovation library page #7196

Conversation

reactoholic
Copy link
Contributor

@reactoholic reactoholic commented Nov 14, 2024

  • Аdd skeleton loading indicators in the innovation library page
  • Fetch innovation packs on page visit rather than using just the cached data - this fixes the problem where the user creates a new innovation flow -> goes to the innovation library page -> not seeing the new pack

Summary by CodeRabbit

  • New Features

    • Introduced loading state management across multiple components, enhancing user experience during data fetching.
    • Added loading indicators using skeleton components in InnovationPacksView and LibraryTemplatesView.
  • Bug Fixes

    • Improved organization and clarity in component structure without altering existing functionalities.
  • Documentation

    • Updated prop interfaces to include optional loading properties for better state management.

Signed-off-by: reactoholic <petar.georgiev.kolev@gmail.com>
Copy link

coderabbitai bot commented Nov 14, 2024

Walkthrough

The changes in this pull request primarily involve the addition of a loading state across several components related to the innovation library. Modifications include the introduction of a loading property in the relevant props interfaces and adjustments to the rendering logic to display loading indicators using Material-UI's Skeleton component. The InnovationLibraryPage component was also updated to manage the loading state from the data fetching operation, ensuring that it is passed down to child components for consistent loading feedback.

Changes

File Change Summary
src/core/ui/card/ContributeCardSkeleton.tsx Removed import statement for React; component implementation remains unchanged.
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx Added optional loading property to DashboardInnovationPacksProps; moved innovationPackValueGetter function to the end of the file.
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx Extended InnovationPacksViewProps with loading property; updated rendering logic to display loading state.
src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx Updated DashboardLibraryTemplatesProps to include loading; modified props destructuring in the component.
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx Added loading prop to LibraryTemplatesViewProps; updated rendering logic to show loading skeleton.
src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx Modified data fetching to include loading state; passed loading prop to DashboardInnovationPacks and DashboardLibraryTemplates.

Possibly related issues

Possibly related PRs

Suggested labels

translations

Suggested reviewers

  • bobbykolev

Warning

Rate limit exceeded

@reactoholic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 55 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning eslint > @humanwhocodes/config-array@0.13.0: Use @eslint/config-array instead
warning eslint > @humanwhocodes/config-array > @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
warning eslint > file-entry-cache > flat-cache > rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning eslint-config-react-app > babel-preset-react-app > @babel/plugin-proposal-class-properties@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
warning eslint-config-react-app > babel-preset-react-app > @babel/plugin-proposal-nullish-coalescing-operator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
warning eslint-config-react-app > babel-preset-react-app > @babel/plugin-proposal-numeric-separator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-numeric-separator instead.
warning eslint-config-react-app > babel-preset-react-app > @babel/plugin-proposal-private-methods@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-private-methods instead.
warning eslint-config-react-app > babel-preset-react-app > @babel/plugin-proposal-optional-chaining@7.21.0: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.


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.

@reactoholic reactoholic linked an issue Nov 14, 2024 that may be closed by this pull request
1 task
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: 4

🧹 Outside diff range and nitpick comments (8)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (2)

17-22: Consider implementing loading state skeleton.

While the loading prop is correctly passed down, consider implementing a skeleton loading state in the InnovationPacksView component to improve user experience during data fetching.

Example implementation:

{loading ? (
  <InnovationPacksSkeleton count={3} />
) : (
  <InnovationPacksView
    filter={filter}
    headerTitle={headerTitle}
    innovationPacks={filteredInnovationPacks.slice(0, MAX_PACKS_WHEN_NOT_EXPANDED)}
    onFilterChange={onFilterChange}
    expanded={isDialogOpen}
    onDialogOpen={() => setIsDialogOpen(true)}
    hasMore={filteredInnovationPacks.length > MAX_PACKS_WHEN_NOT_EXPANDED}
    loading={loading}
  />
)}

Also applies to: 42-42


Line range hint 44-56: Consider propagating loading state to dialog view.

The loading state is not passed to the InnovationPacksView within the dialog. For consistency, consider passing the loading prop here as well.

Apply this change:

  <InnovationPacksView
    filter={filter}
    headerTitle={dialogTitle}
    innovationPacks={filteredInnovationPacks}
    onFilterChange={onFilterChange}
    expanded={isDialogOpen}
    onDialogClose={() => setIsDialogOpen(false)}
    sx={{ flexShrink: 1 }}
+   loading={loading}
  />
src/main/topLevelPages/InnovationLibraryPage/InnovationLibraryPage.tsx (1)

Line range hint 33-41: Consider adding loading state to TopLevelPageLayout

For a better user experience, consider showing a loading state in the page layout when the data is being fetched.

 <TopLevelPageLayout
   title={t('pages.innovationLibrary.title')}
   subtitle={subtitleText}
   iconComponent={InnovationLibraryIcon}
+  loading={innovationLibraryLoading}
   breadcrumbs={
     <TopLevelPageBreadcrumbs>
       <BreadcrumbsItem uri="/innovation-library" iconComponent={InnovationLibraryIcon}>
         {t('pages.innovationLibrary.shortName')}
       </BreadcrumbsItem>
     </TopLevelPageBreadcrumbs>
   }
 >
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (4)

Line range hint 1-13: Consider consolidating Material-UI imports

The imports from @mui/material can be consolidated into a single import statement for better maintainability.

-import { Box, Button, Theme, useMediaQuery } from '@mui/material';
-import { Skeleton } from '@mui/material';
+import { Box, Button, Skeleton, Theme, useMediaQuery } from '@mui/material';

Line range hint 27-38: Add default value for loading prop

Consider adding a default value for the loading prop to avoid undefined checks in the render logic.

const InnovationPacksView = ({
  headerTitle,
  innovationPacks,
  filter,
  onFilterChange,
  expanded = false,
  onDialogOpen,
  onDialogClose,
  hasMore = false,
-  loading,
+  loading = false,
  ...props
}: InnovationPacksViewProps) => {

53-64: Extract magic numbers into named constants

The skeleton implementation uses magic numbers for count and dimensions. Consider extracting these into named constants for better maintainability.

+const SKELETON_COUNT = 5;
+const SKELETON_WIDTH = 240;
+const SKELETON_HEIGHT = 300;

-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 }}
   />
))

Filter should respect loading state for consistent UX

The component already handles loading state for cards by showing skeletons, but the filter remains fully interactive. To maintain consistency:

  • Add the disabled prop to the MultipleSelect component when loading is true
  • Location: src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx

Example:

actions={
  <MultipleSelect 
    onChange={onFilterChange} 
    value={filter} 
    minLength={2} 
    size="xsmall" 
    inlineTerms
    disabled={loading}
  />
}
🔗 Analysis chain

Line range hint 44-50: Consider filter visibility during loading state

The filter component is always visible regardless of the loading state. Consider if it should be disabled or hidden during loading.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other components in the codebase handle filter visibility during loading
ast-grep --pattern 'loading ? $_ : <MultipleSelect'

Length of output: 53


Script:

#!/bin/bash
# Let's search for patterns where MultipleSelect is used with loading states
rg -U "loading.*MultipleSelect|MultipleSelect.*loading" -A 3 -B 3

# Also check for any loading patterns in the specific file
rg -U "loading" "src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx" -A 3 -B 3

# Search for similar filter components and their loading handling
ast-grep --pattern '<$_Select $$$loading$$$>'

Length of output: 744

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

Line range hint 22-33: Consider using interface instead of type alias for props

While the type alias works, using an interface would provide better extensibility through inheritance. This is particularly useful for component props that might need to be extended in the future.

-type LibraryTemplatesViewProps = {
+interface LibraryTemplatesViewProps {
  filter: LibraryTemplatesFilter;
  // ... other props
  loading?: boolean;
-};
+}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

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

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

8-13: LGTM! Clean type definition with proper optional prop.

The type definition follows TypeScript best practices with clear prop types and optional loading state.


62-67: Good function extraction for better maintainability.

Moving innovationPackValueGetter out of the component body improves code organization and reusability. The function is pure and has clear type definitions.

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

56-56: Verify skeleton loading implementation in DashboardLibraryTemplates

The loading prop is correctly passed down, but let's verify its implementation in the child component.

#!/bin/bash
# Description: Check if DashboardLibraryTemplates properly implements skeleton loading

# Check for skeleton component imports and usage
rg -A 5 "DashboardLibraryTemplates.*loading.*props" src/domain/InnovationPack/DashboardLibraryTemplates/
rg "Skeleton" src/domain/InnovationPack/DashboardLibraryTemplates/

49-49: Verify skeleton loading implementation in DashboardInnovationPacks

The loading prop is correctly passed down, but let's verify its implementation in the child component.

✅ Verification successful

Let me gather more information to verify the skeleton loading implementation.


Let me check the actual implementation of the loading prop and skeleton usage in the InnovationPacksView component.


Loading prop is correctly implemented with skeleton loading

The loading prop is properly handled in the InnovationPacksView component, which renders a loading skeleton state when loading is true. The implementation shows:

  • Correct usage of MUI Skeleton component
  • Appropriate loading state handling with conditional rendering
  • Proper skeleton dimensions and animation for a smooth user experience
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if DashboardInnovationPacks properly implements skeleton loading

# Check for skeleton component imports and usage
rg -A 5 "DashboardInnovationPacks.*loading.*props" src/domain/InnovationPack/DashboardInnovationPacks/
rg "Skeleton" src/domain/InnovationPack/DashboardInnovationPacks/

Length of output: 428


Script:

#!/bin/bash
# Search for DashboardInnovationPacks component definition and props
ast-grep --pattern 'interface DashboardInnovationPacksProps {
  $$$
}'

# Check the component implementation
ast-grep --pattern 'const DashboardInnovationPacks = ({ $$ }: DashboardInnovationPacksProps) => {
  $$$
}'

# Search for Skeleton imports and usage in the component
rg -A 2 "import.*Skeleton" src/domain/InnovationPack/DashboardInnovationPacks/
rg "Skeleton" src/domain/InnovationPack/DashboardInnovationPacks/

Length of output: 879


Script:

#!/bin/bash
# Check the InnovationPacksView component implementation focusing on loading prop and Skeleton usage
ast-grep --pattern 'interface InnovationPacksViewProps {
  $$$
}'

# Search for the component implementation with context
rg -A 10 "const InnovationPacksView.*=.*{" src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx

# Search for Skeleton usage with more context
rg -B 5 -A 5 "<Skeleton" src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx

Length of output: 789

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

9-14: LGTM! Type definition changes look good.

The addition of the optional loading prop aligns well with the PR objective of implementing skeleton loading indicators.


28-33: LGTM! Parameter destructuring is well organized.

The loading prop is appropriately placed at the beginning of the parameter list, following common patterns for status-related props.


73-73: Verify loading state handling in dialog view.

The loading prop is passed to the main LibraryTemplatesView but not to the one inside DialogWithGrid. This could lead to inconsistent loading states between the main view and dialog view.

Let's check if LibraryTemplatesView handles loading state consistently:

Also applies to: 76-83

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

Line range hint 34-47: LGTM! Props destructuring is well implemented

The component parameters are properly typed and destructured, with good use of the Omit utility type to handle prop inheritance.

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: 3

🧹 Outside diff range and nitpick comments (7)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (2)

Line range hint 46-54: Add loading prop to dialog view for consistency.

The loading prop is passed to the main InnovationPacksView but not to the one in the dialog. This could lead to inconsistent loading states between the two views.

Apply this diff to fix the issue:

 <InnovationPacksView
   filter={filter}
   headerTitle={dialogTitle}
   innovationPacks={filteredInnovationPacks}
   onFilterChange={onFilterChange}
   expanded={isDialogOpen}
   onDialogClose={() => setIsDialogOpen(false)}
   sx={{ flexShrink: 1 }}
+  loading={loading}
 />

62-67: Consider memoizing the value getter function.

The innovationPackValueGetter function is used in a useMemo hook, but the function itself is recreated on every render. Consider moving it outside the component or using useCallback to memoize it.

Here's how you could optimize it:

+const innovationPackValueGetter = useCallback(
   (innovationPack: Identifiable & InnovationPackCardProps): ValueType => ({
     id: innovationPack.id,
     values: [innovationPack.displayName, ...(innovationPack.tags ?? [])],
   }),
+  []
+);
src/domain/InnovationPack/DashboardLibraryTemplates/DashboardLibraryTemplates.tsx (2)

9-14: LGTM! Consider adding JSDoc comments

The type definition is well-structured with proper typing. The optional loading prop is correctly typed as boolean.

Consider adding JSDoc comments to document the purpose of each prop:

+/**
+ * Props for the DashboardLibraryTemplates component
+ * @property headerTitle - Title to display in the header
+ * @property dialogTitle - Title to display in the dialog
+ * @property templates - Array of templates with their associated innovation packs
+ * @property loading - Whether the templates are currently being loaded
+ */
type DashboardLibraryTemplatesProps = {
  headerTitle: ReactNode;
  dialogTitle: ReactNode;
  templates: AnyTemplateWithInnovationPack[] | undefined;
  loading?: boolean;
};

28-33: Consider maintaining consistent prop ordering

The order of props in the destructuring differs from their order in the type definition. Consider maintaining the same order for better maintainability.

-const DashboardLibraryTemplates = ({
-  loading,
-  templates,
-  headerTitle,
-  dialogTitle,
-}: DashboardLibraryTemplatesProps) => {
+const DashboardLibraryTemplates = ({
+  headerTitle,
+  dialogTitle,
+  templates,
+  loading,
+}: DashboardLibraryTemplatesProps) => {
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (2)

53-64: Consider extracting skeleton count to a constant

The hardcoded value of 5 skeleton items might not match the actual number of visible items in different viewport sizes.

+const VISIBLE_ITEMS_COUNT = 5; // TODO: Adjust based on typical viewport size
 {loading
-  ? Array.from({ length: 5 }).map((_, idx) => (
+  ? Array.from({ length: VISIBLE_ITEMS_COUNT }).map((_, idx) => (

55-62: Consider using theme spacing for dimensions

The Skeleton dimensions are using magic numbers. Consider using theme spacing or extracting these values to constants.

 <Skeleton
   key={idx}
-  width={240}
-  height={300}
+  sx={{
+    width: theme => theme.spacing(30), // 240px
+    height: theme => theme.spacing(37.5), // 300px
+    borderRadius: 1
+  }}
   animation="pulse"
   variant="rectangular"
-  sx={{ borderRadius: 1 }}
 />
src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx (1)

91-111: Consider improving the skeleton loading implementation.

While the current implementation works, here are some suggestions for improvement:

  1. The number of skeleton items is hardcoded to 5. Consider making this dynamic based on the expected number of items or the typical number of visible items.
  2. The skeleton dimensions (248x262) are hardcoded. Consider extracting these as constants or deriving them from the actual card dimensions to maintain consistency.
+ const TEMPLATE_CARD_WIDTH = 248;
+ const TEMPLATE_CARD_HEIGHT = 262;
+ const SKELETON_COUNT = 5; // or calculate based on viewport

  <ScrollableCardsLayoutContainer minHeight={0} orientation={expanded ? 'vertical' : undefined} sameHeight>
    {loading
-     ? Array.from({ length: 5 }).map((_, idx) => (
+     ? Array.from({ length: SKELETON_COUNT }).map((_, idx) => (
          <Skeleton
            key={idx}
-           width={248}
-           height={262}
+           width={TEMPLATE_CARD_WIDTH}
+           height={TEMPLATE_CARD_HEIGHT}
            animation="pulse"
            variant="rectangular"
            sx={{ borderRadius: 1 }}
          />
        ))
      : templates?.map(({ template, innovationPack }) => (
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

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

📒 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 (8)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (1)

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

The change from interface to type and the addition of the optional loading prop are well-structured.

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

49-49: LGTM! Loading state properly propagated.

The loading prop is correctly passed to DashboardInnovationPacks, enabling skeleton loading indicators during data fetching.


56-56: LGTM! Consistent loading state handling.

The loading prop is correctly passed to DashboardLibraryTemplates, maintaining consistency in loading state handling across components.


17-19: LGTM! Verify the network-only fetch policy impact.

The addition of loading state and network-only fetch policy aligns well with the requirement to show newly created packs. This ensures fresh data is always fetched when visiting the page.

✅ Verification successful

Network-only fetch policy is consistent with similar use cases

The network-only fetch policy usage in InnovationLibraryPage aligns with the established pattern across the codebase. Similar components like MyDashboard, ChatWidget, and other data-sensitive views also use network-only to ensure fresh data. This change maintains consistency with the existing architecture.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if there are any other queries in the codebase that might need similar treatment
# for consistency in fetching innovation packs

# Search for innovation pack related queries
rg -l "useInnovationLibraryQuery|InnovationLibraryQuery" --type ts --type tsx

# Search for other network-only policies to ensure consistency
rg "fetchPolicy:.+network-only" --type ts --type tsx

Length of output: 188


Script:

#!/bin/bash
# Let's try without file type filters since tsx is not recognized
# Search for innovation pack related queries
rg -l "useInnovationLibraryQuery|InnovationLibraryQuery"

# Search for network-only fetch policies
rg "fetchPolicy.*network-only"

Length of output: 1514

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

13-13: LGTM: Skeleton import is properly placed

The Skeleton component import is correctly grouped with other Material-UI imports.


Line range hint 15-24: LGTM: Interface extension follows TypeScript best practices

The loading prop is correctly added as an optional boolean property, maintaining backward compatibility while extending the interface properly.

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

Line range hint 22-33: LGTM! Clean type definition for loading state.

The optional loading prop is well-typed and follows TypeScript best practices.


91-128: LGTM! Clean implementation of skeleton loading.

The implementation successfully achieves the PR objective of adding skeleton loading indicators while maintaining existing functionality:

  • Proper loading state management
  • Responsive design maintained
  • Filter functionality preserved
  • Smooth transition between loading and loaded states

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: 3

🧹 Outside diff range and nitpick comments (2)
src/domain/InnovationPack/DashboardInnovationPacks/DashboardInnovationPacks.tsx (1)

Line range hint 27-30: Consider additional memoization for performance

While filteredInnovationPacks is memoized, the component could benefit from memoizing the sliced array to prevent unnecessary re-renders in child components.

Consider applying this change:

 const filteredInnovationPacks = useMemo(
   () => filterFn(innovationPacks ?? [], filter, innovationPackValueGetter),
   [innovationPacks, filter]
 );
+
+const displayedInnovationPacks = useMemo(
+  () => filteredInnovationPacks.slice(0, MAX_PACKS_WHEN_NOT_EXPANDED),
+  [filteredInnovationPacks]
+);

Then update the InnovationPacksView prop:

-innovationPacks={filteredInnovationPacks.slice(0, MAX_PACKS_WHEN_NOT_EXPANDED)}
+innovationPacks={displayedInnovationPacks}
src/domain/InnovationPack/DashboardInnovationPacks/InnovationPacksView.tsx (1)

53-64: Consider making skeleton dimensions dynamic.

While the loading state implementation is good, the hardcoded skeleton dimensions (240x300) might not match the actual InnovationPackCard dimensions in all viewport sizes.

Consider this approach:

-<Skeleton
-  key={idx}
-  width={240}
-  height={300}
-  animation="pulse"
-  variant="rectangular"
-  sx={{ borderRadius: 1 }}
-/>
+<Box sx={{ width: { xs: '100%', sm: 240 } }}>
+  <Skeleton
+    key={idx}
+    width="100%"
+    height={300}
+    animation="pulse"
+    variant="rectangular"
+    sx={{ borderRadius: 1 }}
+  />
+</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 d397327.

📒 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! Props are well-defined with proper TypeScript types

The addition of the optional loading prop and the conversion to a type definition maintains type safety while adding the necessary loading state support.


62-67: LGTM! Well-structured helper function

Moving innovationPackValueGetter to a standalone function improves code organization while maintaining proper type safety and null handling.

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

49-49: LGTM! Loading state properly implemented for innovation packs.

The loading prop is correctly passed down to enable skeleton loading indicators.


56-56: LGTM! Loading state properly implemented for library templates.

The loading prop is correctly passed down to enable skeleton loading indicators.


17-19: LGTM! The network-only fetch policy addresses the caching issue.

The change ensures fresh data is fetched on each page visit, solving the issue where newly created innovation packs weren't immediately visible.

Let's verify there are no other instances relying on cached data:

✅ Verification successful

Let me search for any other potential usages of this query hook to ensure we haven't missed anything.


The network-only fetch policy implementation is correctly scoped

The query hook useInnovationLibraryQuery is only used in InnovationLibraryPage.tsx and the implementation of the network-only fetch policy is appropriate for this single usage. The generated hooks file confirms this is the only implementation point.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other innovation library queries that might need the same fetch policy
ast-grep --pattern 'useInnovationLibraryQuery({
  $$$
})'

Length of output: 425


Script:

#!/bin/bash
# Search for any imports of useInnovationLibraryQuery
rg "useInnovationLibraryQuery" -A 2

Length of output: 2495

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

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

The type declaration follows TypeScript best practices with proper typing of the optional loading prop.


28-33: LGTM! Clean props destructuring.

The props destructuring is well-organized and maintains consistency with the type declaration.


73-73: Verify loading state handling in LibraryTemplatesView.

The loading prop is correctly passed down, but we should verify that LibraryTemplatesView properly implements skeleton loading indicators as per PR objectives.

✅ Verification successful

Let me gather more information about the loading state implementation.


Loading state is properly implemented in LibraryTemplatesView

The component correctly implements loading state with skeleton placeholders:

  • Uses MUI Skeleton component for loading indicators
  • Renders 5 skeleton items during loading state
  • Each skeleton has appropriate dimensions (248x262) and styling (rounded corners, pulse animation)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify LibraryTemplatesView implementation of loading state
# Expected: Should find Skeleton component usage in LibraryTemplatesView

# Search for Skeleton imports and usage in LibraryTemplatesView
ast-grep --pattern 'import { $$$, Skeleton, $$$ } from "@mui/material"' 

# Search for loading prop handling
rg -A 5 'loading.*\?' src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx

Length of output: 259


Script:

#!/bin/bash
# Search for any loading-related rendering logic in LibraryTemplatesView
rg -A 10 'loading' src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx

# Search for any skeleton or loading placeholder components
ast-grep --pattern 'return (
  $$$
  {loading ? (
    $$$
  ) : (
    $$$
  )}
  $$$
)'

# Check if there are any other skeleton components in the file
rg -i 'skeleton' src/domain/InnovationPack/DashboardLibraryTemplates/LibraryTemplatesView.tsx

Length of output: 1245

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

Line range hint 13-24: LGTM! Clean interface extension and import addition.

The interface extension and new import are well-structured and properly typed.


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

The component signature is clean and properly handles all props including the new loading state.

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

1-2: LGTM! Type definitions and imports are well structured.

The changes follow TypeScript best practices with proper typing and necessary imports.

Also applies to: 22-33


Line range hint 34-47: LGTM! Props destructuring is clean and consistent.

The loading prop is properly integrated into the existing props structure.

@reactoholic reactoholic requested review from ccanos and removed request for bobbykolev November 14, 2024 11:36
@reactoholic
Copy link
Contributor Author

I will close this PR, delete my remote branch and recreate it, thus reopen the PR, because currently git is rejecting my resolved comments.

@reactoholic reactoholic deleted the client-web/7174-loading-indicator-missing-for-innovation-library branch November 14, 2024 11:56
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.

Loading indicator missing for Innovation Library
4 participants