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

Use Session Storage to hold temp state for DataGrid #1618

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

rdonigian
Copy link
Contributor

@rdonigian rdonigian commented Nov 5, 2024

Monday

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced session storage functionality for grid state management across multiple components, enhancing user experience by retaining configurations.
    • Added a new ProgressReportsToolbar for improved interaction in the ProgressReportsExpandedGrid.
    • Enhanced ProgressReportsGrid to manage its state through session storage, improving configurability.
  • Bug Fixes

    • Improved scrolling behavior in the DataGrid during sorting and filtering operations.
  • Documentation

    • Updated usage documentation for new properties and hooks related to session storage integration.

@rdonigian rdonigian requested a review from CarsonF as a code owner November 5, 2024 19:30
Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces several enhancements to the grid components across the application, primarily focusing on state management through session storage. The useDataGridSource hook has been updated to accept a new sessionStorageProps parameter, allowing for the persistence of grid states. New components and modifications to existing components, such as ProgressReportsGrid and ProgressReportsExpandedGrid, incorporate this session storage functionality, improving user experience by retaining grid configurations across sessions.

Changes

File Change Summary
src/components/Grid/useDataGridSource.tsx Added import for GridInitialStatePro and usePersistedGridState. Updated function signature to include sessionStorageProps. Enhanced internal logic with savedGridState. Modified onSortModelChange and onFilterModelChange functions.
src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx Introduced ProgressReportsToolbar component. Defined initialState object and updated slots to include the new toolbar. Modified props to omit columns.
src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx Added sessionStorageProps to useDataGridSource with partnerId and EngagementInitialState. Removed initialState from DataGrid.
src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx Added sessionStorageProps to useDataGridSource with partnerId and ProjectInitialState. Removed initialState from DataGrid.
src/scenes/Projects/List/EngagementsPanel.tsx Added sessionStorageProps to useDataGridSource with key 'engagements-grid' and EngagementInitialState. Removed initialState from DataGrid.
src/scenes/Projects/List/ProjectsPanel.tsx Added sessionStorageProps to useDataGridSource with key 'projects-grid' and ProjectInitialState. Removed hardcoded initialState from DataGrid.
src/hooks/usePersistedGridState.tsx Introduced usePersistedGridState hook for managing grid state with session storage. Accepts key, apiRef, and defaultValue.
src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsGrid.tsx Updated function signature to include initialState. Modified useDataGridSource call to include sessionStorageProps with key 'progress-reports-grid'.

Possibly related PRs

  • Deep merge User & System Filters for DataGrid #1616: The changes in this PR also modify the useDataGridSource hook, indicating a direct relationship with the main PR's updates to the same hook, although the focus is on merging variables rather than session storage management.

Suggested reviewers

  • CarsonF

Poem

In the grid where data flows,
A rabbit hops where session grows.
With states that save, and grids that gleam,
Our toolbar shines, a user’s dream!
So let us cheer, with joyful thumps,
For grids that remember, and never slump! 🐇✨


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

🧹 Outside diff range and nitpick comments (7)
src/hooks/useSessionStorage.tsx (2)

21-27: Optimize storage updates and prevent race conditions.

The current implementation has duplicated storage logic and potential race conditions.

Consider extracting the storage logic and implementing debouncing:

+import { useMemo } from 'react';
+import debounce from 'lodash/debounce';
+
+const updateStorage = (key: string, value: unknown) => {
+  if (typeof window !== 'undefined') {
+    try {
+      sessionStorage.setItem(key, JSON.stringify(value));
+    } catch (error) {
+      console.error('Failed to save to session storage:', error);
+    }
+  }
+};

 const setValue = (value: T | ((val: T) => T)) => {
   const valueToStore = value instanceof Function ? value(storedValue) : value;
   setStoredValue(valueToStore);
-  if (typeof window !== 'undefined') {
-    sessionStorage.setItem(storageKey, JSON.stringify(valueToStore));
-  }
 };

+const debouncedUpdate = useMemo(
+  () => debounce((value: T) => updateStorage(storageKey, value), 300),
+  [storageKey]
+);
+
+useEffect(() => {
+  debouncedUpdate(storedValue);
+  return () => {
+    debouncedUpdate.cancel();
+  };
+}, [debouncedUpdate, storedValue]);

29-30: Consider adding reset functionality and explicit return type.

The hook could benefit from additional utility functions and explicit typing.

Consider this enhancement:

+type StorageHookResult<T> = readonly [
+  T,
+  (value: T | ((val: T) => T)) => void,
+  () => void
+];
+
-return [storedValue, setValue] as const;
+const reset = () => {
+  setStoredValue(defaultValue);
+  if (typeof window !== 'undefined') {
+    sessionStorage.removeItem(storageKey);
+  }
+};
+
+return [storedValue, setValue, reset] as StorageHookResult<T>;
src/scenes/Projects/List/ProjectsPanel.tsx (2)

26-31: Consider making the storage key more specific

The current storage key 'projects-grid-state' might be too generic if this component is used in different contexts (e.g., different routes or views). Consider making it more specific or dynamic.

- 'projects-grid-state',
+ `projects-grid-state-${context}`,  // where context could be route/view identifier

77-77: Consider memoizing the sx prop array

The sx prop array is recreated on every render. Consider memoizing it with useMemo to prevent unnecessary re-renders.

+ const gridSx = useMemo(
+   () => [flexLayout, noHeaderFilterButtons, noFooter],
+   []
+ );

  return (
    <DataGrid<Project>
      {...DefaultDataGridStyles}
      {...dataGridProps}
      slots={slots}
      slotProps={slotProps}
      columns={ProjectColumns}
      initialState={savedGridState}
      headerFilters
      hideFooter
      onStateChange={onStateChange.run}
-     sx={[flexLayout, noHeaderFilterButtons, noFooter]}
+     sx={gridSx}
    />
  );

Also applies to: 80-81

src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx (1)

63-72: Consider performance optimizations for the state change handler.

While the current implementation is functional, consider these improvements:

  1. The maxWait being equal to wait might not provide enough protection against rapid updates.
  2. The function could be memoized to prevent unnecessary recreations.

Consider this optimization:

-  const onStateChange = useDebounceFn(
+  const onStateChange = useDebounceFn(
+    useCallback(
     () => {
       const gridState = props.apiRef.current.exportState();
       if (!isEqual(gridState, prevState.current)) {
         prevState.current = gridState;
         setSavedGridState(gridState);
       }
-    },
-    { wait: 500, maxWait: 500 }
+    },
+    [props.apiRef, setSavedGridState]
+    ),
+    { wait: 500, maxWait: 1000 }
   );
src/components/Grid/useDataGridSource.tsx (1)

393-396: LGTM! Consider queueing scroll operations.

The mount check is a good defensive programming practice. However, consider queueing the scroll operation for when the grid becomes available, especially if maintaining scroll position is critical for UX.

Example enhancement:

-if (apiRef.current.rootElementRef.current) {
-  apiRef.current.scrollToIndexes({ rowIndex: 0 });
-}
+const scrollToTop = () => {
+  if (apiRef.current.rootElementRef.current) {
+    apiRef.current.scrollToIndexes({ rowIndex: 0 });
+  } else {
+    // Queue scroll operation for next render
+    requestAnimationFrame(scrollToTop);
+  }
+};
+scrollToTop();
src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx (1)

141-150: Optimize state comparison in onStateChange

The use of isEqual to compare gridState with prevState.current can be computationally expensive for large state objects. Consider optimizing the comparison by checking only the parts of the state that are prone to change or implementing a more efficient comparison mechanism to improve performance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cd2336c and 9302418.

📒 Files selected for processing (7)
  • src/components/Grid/useDataGridSource.tsx (2 hunks)
  • src/hooks/useSessionStorage.tsx (1 hunks)
  • src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx (5 hunks)
  • src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx (4 hunks)
  • src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx (4 hunks)
  • src/scenes/Projects/List/EngagementsPanel.tsx (3 hunks)
  • src/scenes/Projects/List/ProjectsPanel.tsx (3 hunks)
🔇 Additional comments (16)
src/scenes/Projects/List/ProjectsPanel.tsx (1)

5-8: LGTM: Clean import additions

The new imports are well-organized and properly scoped for the grid state management implementation.

Also applies to: 22-22

src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx (4)

6-8: LGTM: Appropriate imports added for grid state management.

The new imports provide the necessary types and utilities for implementing grid state persistence.

Also applies to: 28-28


41-41: LGTM: Appropriate use of useRef for previous state tracking.

The implementation correctly uses useRef to track the previous state without causing re-renders.


89-89: LGTM: Proper integration with DataGrid component.

The onStateChange handler is correctly connected to the DataGrid component.


36-40: LGTM: Well-implemented session storage with proper isolation.

The implementation correctly isolates grid state per partner and maintains type safety.

Let's verify the ProjectInitialState import and structure:

✅ Verification successful

LGTM: Session storage implementation is correctly using a well-defined initial state

The implementation is properly using ProjectInitialState which is a type-safe grid configuration that:

  • Pins the first column (name) to the left
  • Sets default column visibility with some columns hidden by default (isMember, pinned)
  • Satisfies the DataGridProps['initialState'] type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the structure of ProjectInitialState
ast-grep --pattern 'const ProjectInitialState = {
  $$$
}'

Length of output: 60


Script:

#!/bin/bash
# Let's try a broader search for ProjectInitialState
rg "ProjectInitialState" -A 5

# Also search for any grid state initialization patterns
ast-grep --pattern 'const $_ = {
  sortModel: $_,
  filterModel: $_,
  $$$
}'

Length of output: 3294


Script:

#!/bin/bash
# Let's check the actual structure of ProjectInitialState
cat src/components/ProjectDataGrid/ProjectColumns.tsx

Length of output: 3975

src/components/Grid/useDataGridSource.tsx (1)

Line range hint 393-418: Consider comprehensive error handling for grid operations.

While the mount checks improve reliability, consider implementing a more comprehensive error handling strategy for grid operations. This could include:

  1. Logging failed operations for debugging
  2. Retrying failed operations where appropriate
  3. Providing user feedback for failed interactions
  4. Handling edge cases like rapid sort/filter changes

This would make the grid more resilient to various failure modes and provide a better debugging experience.

Let's verify the current error handling coverage:

src/scenes/Projects/List/EngagementsPanel.tsx (6)

5-7: Imports are correctly integrated

The necessary modules (GridInitialStatePro, useDebounceFn, isEqual, merge) are imported and utilized appropriately in the code.


22-22: Session storage hook is imported properly

The useSessionStorage hook is imported from '~/hooks/useSessionStorage' and is ready for use in managing the grid state persistence.


26-31: Grid state persistence is effectively implemented

The savedGridState and setSavedGridState state variables are correctly initialized using useSessionStorage, and prevState is set up to track the previous grid state using useRef.


55-65: Debounced state change handler is properly implemented

The onStateChange function uses useDebounceFn to debounce the grid state changes, minimizing unnecessary updates to session storage. This enhances performance by reducing the frequency of state saves.


66-69: Grid state restoration is handled correctly

The useEffect hook restores the grid state from savedGridState when it changes, ensuring the grid reflects the persisted state upon re-rendering. The dependency array is accurately specified.


77-80: DataGrid component updated with persistent state management

The DataGrid component's initialState prop is updated to use savedGridState, and the onStateChange prop is linked to the debounced onStateChange.run function, enabling state persistence across sessions.

src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx (1)

6-6: Ensure ahooks is included in project dependencies.

Please verify that ahooks is added to your package.json dependencies to ensure it is properly installed and available.

Run the following script to confirm that ahooks is listed in package.json dependencies:

✅ Verification successful

ahooks dependency is correctly included in package.json

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if 'ahooks' is listed in package.json dependencies.

grep -q '"ahooks"' package.json && echo "'ahooks' is present in dependencies." || echo "'ahooks' is missing from dependencies."

Length of output: 122

src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx (3)

10-10: Confirm the necessity of new external dependencies

The imports of useDebounceFn from ahooks and isEqual from lodash introduce new external dependencies to the project. Please verify whether adding these libraries is essential for the implementation. If possible, consider using native React hooks or utilities to minimize dependency bloat and potential maintenance overhead.

Also applies to: 12-14, 24-24


152-155: Verify the impact of restoring state in useEffect

Restoring the grid state using apiRef.current.restoreState(savedGridState) inside a useEffect hook may lead to unintended side effects, such as unnecessary re-renders or conflicts with other grid operations. Please ensure that this implementation does not adversely affect the component's behavior.

If you'd like assistance in reviewing this implementation or exploring alternative solutions, I'm happy to help.


123-128: ⚠️ Potential issue

Ensure proper serialization in useSessionStorage

When storing complex objects like GridInitialStatePro in session storage, it's important to ensure that serialization and deserialization are handled correctly. Please confirm that the useSessionStorage hook manages this process without data loss or corruption.

src/hooks/useSessionStorage.tsx Outdated Show resolved Hide resolved
src/hooks/useSessionStorage.tsx Outdated Show resolved Hide resolved
src/hooks/useSessionStorage.tsx Outdated Show resolved Hide resolved
src/scenes/Projects/List/ProjectsPanel.tsx Outdated Show resolved Hide resolved
src/scenes/Projects/List/ProjectsPanel.tsx Outdated Show resolved Hide resolved
src/components/Grid/useDataGridSource.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: 2

🧹 Outside diff range and nitpick comments (4)
src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx (1)

34-37: Consider memoizing sessionStorageProps.

The sessionStorageProps object is recreated on every render. While this might not cause performance issues due to the stable partnerId, it's a good practice to memoize objects passed to hooks.

Consider this improvement:

+ const sessionStorageProps = useMemo(
+   () => ({
+     key: `partners-engagements-grid-state-${partnerId}`,
+     defaultValue: EngagementInitialState,
+   }),
+   [partnerId]
+ );

  const [props] = useDataGridSource({
    query: PartnerDetailEngagementsDocument,
    variables: { id: partnerId },
    listAt: 'partner.engagements',
    initialInput: {
      sort: EngagementColumns[0]!.field,
    },
-   sessionStorageProps: {
-     key: `partners-engagements-grid-state-${partnerId}`,
-     defaultValue: EngagementInitialState,
-   },
+   sessionStorageProps,
  });
src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsGrid.tsx (1)

180-180: LGTM! Consider adding type annotation for initialState.

The addition of the initialState parameter with a default empty object is a good approach for optional state initialization.

Consider adding explicit type annotation for better type safety:

-  initialState = {},
+  initialState: Partial<DataGridProps['initialState']> = {},
src/components/Grid/useDataGridSource.tsx (2)

428-432: Add JSDoc documentation for session storage integration.

The session storage integration looks good, but would benefit from documentation explaining:

  • What grid state properties are being persisted
  • How the persistence behavior interacts with the existing local storage functionality
  • Any cleanup/expiration strategy for the session storage

Example documentation:

+/**
+ * Persists grid state (e.g., column widths, visibility) in session storage.
+ * Note: Sorting and filtering are handled separately via local storage.
+ * @param key - Unique identifier for the grid state in session storage
+ * @param defaultValue - Default grid state if nothing exists in session storage
+ */
 const [savedGridState = {}, onStateChange] = usePersistedGridState({
   key: sessionStateProps.key,
   apiRef: apiRef,
   defaultValue: sessionStateProps.defaultValue,
 });

Also applies to: 449-449, 454-454


428-432: Document the dual storage strategy.

The component now uses both local storage (for view state like sorting/filtering) and session storage (for grid state). This dual storage approach needs clarification:

  1. What is the rationale for using different storage mechanisms?
  2. Are there any potential synchronization issues between the two storage types?
  3. How should other developers decide which storage to use for new state properties?

Consider adding architectural documentation to explain these decisions.

Also applies to: 449-449, 454-454

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9302418 and cf430c7.

📒 Files selected for processing (8)
  • src/components/Grid/useDataGridSource.tsx (6 hunks)
  • src/hooks/usePersistedGridState.tsx (1 hunks)
  • src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx (0 hunks)
  • src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsGrid.tsx (2 hunks)
  • src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx (1 hunks)
  • src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx (1 hunks)
  • src/scenes/Projects/List/EngagementsPanel.tsx (1 hunks)
  • src/scenes/Projects/List/ProjectsPanel.tsx (1 hunks)
💤 Files with no reviewable changes (1)
  • src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsExpandedGrid.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/scenes/Partners/Detail/Tabs/Projects/PartnerDetailProjects.tsx
  • src/scenes/Projects/List/EngagementsPanel.tsx
  • src/scenes/Projects/List/ProjectsPanel.tsx
🔇 Additional comments (4)
src/scenes/Partners/Detail/Tabs/Engagements/PartnerDetailEngagements.tsx (1)

34-37: Validate partnerId before using in storage key.

The storage key uses partnerId which could be an empty string from useParams(). This might lead to conflicts if multiple partner detail pages are opened without an ID.

Let's verify the partner ID handling in similar components:

src/scenes/Dashboard/ProgressReportsWidget/ProgressReportsGrid.tsx (1)

218-221: Consider storage key management and data size implications.

The session storage implementation looks good, but consider these suggestions:

  1. Move the storage key to a constants file to maintain consistency and prevent typos
  2. Consider implementing size limits or cleanup for the stored state to prevent performance issues with large datasets

Let's verify the storage key usage and potential data size:

src/components/Grid/useDataGridSource.tsx (2)

21-21: LGTM: Type definitions and imports are well-structured.

The new imports and type definitions for session storage integration are properly organized and strongly typed.

Also applies to: 43-43, 104-115


400-403: 🛠️ Refactor suggestion

Extract duplicate scroll behavior.

The scroll-to-top logic is duplicated between onSortModelChange and onFilterModelChange. While the mount check fixes the immediate issue, we should still extract this into a reusable function.

+const scrollGridToTopIfMounted = (apiRef: MutableRefObject<GridApiPro>) => {
+  if (apiRef.current.rootElementRef.current) {
+    apiRef.current.scrollToIndexes({ rowIndex: 0 });
+  }
+};

 const onSortModelChange: DataGridProps['onSortModelChange'] & {} =
   useMemoizedFn((next) => {
     // ... existing sort logic ...
-    if (apiRef.current.rootElementRef.current) {
-      apiRef.current.scrollToIndexes({ rowIndex: 0 });
-    }
+    scrollGridToTopIfMounted(apiRef);
   });

 const onFilterModelChange: DataGridProps['onFilterModelChange'] & {} =
   useMemoizedFn((filterModel) => {
     // ... existing filter logic ...
-    if (apiRef.current.rootElementRef.current) {
-      apiRef.current.scrollToIndexes({ rowIndex: 0 });
-    }
+    scrollGridToTopIfMounted(apiRef);
   });

Also applies to: 422-425

src/hooks/usePersistedGridState.tsx Outdated Show resolved Hide resolved
Comment on lines +22 to +25
const prevGridState = usePrevious(
savedGridState,
(prev, next) => !isEqual(prev, next)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect usage of the compare function in usePrevious

The compare function in usePrevious should return true to keep the previous value unchanged when the values are equal. Currently, it returns true when the values are not equal, which prevents prevGridState from updating correctly. Adjust the compare function to isEqual(prev, next) to ensure that prevGridState updates only when the values differ.

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 (5)
src/hooks/usePersistedGridState.tsx (3)

25-26: Consider using a more robust key generation method

The string concatenation for the filter model key could lead to conflicts. Consider using a more structured approach.

-`${key}-api-filter`
+`grid-${key}-filter-model`

33-50: Consider making debounce configuration configurable

The debounce wait times are hardcoded. Consider making them configurable through options.

+interface UsePersistedGridStateOptions {
+  key: string;
+  apiRef: MutableRefObject<GridApiPro>;
+  defaultValue: GridInitialStatePro;
+  debounceWait?: number;
+}

 const { run: handleStateChange } = useDebounceFn(
   () => {
     // ...
   },
-  { wait: 500, maxWait: 500 }
+  { wait: debounceWait ?? 500, maxWait: debounceWait ?? 500 }
 );

Improve readability of filter model update logic

The nested ternary operation makes the code harder to read. Consider restructuring for clarity.

-setPersistedFilterModel((prev) =>
-  isEqual(prev, gridState)
-    ? prev
-    : convertMuiFiltersToApi(
-        apiRef.current,
-        gridState.filter?.filterModel
-      )
-);
+setPersistedFilterModel((prev) => {
+  const newFilterModel = convertMuiFiltersToApi(
+    apiRef.current,
+    gridState.filter?.filterModel
+  );
+  return isEqual(prev, gridState) ? prev : newFilterModel;
+});

64-65: Consider adding explicit return type

While the as const assertion works, an explicit return type would make the API more clear.

+type PersistedGridStateReturn = readonly [
+  GridInitialStatePro,
+  GridEventListener<'stateChange'>,
+  Record<string, any>
+];

-return [savedGridState, onStateChange, persistedFilterModel] as const;
+return [savedGridState, onStateChange, persistedFilterModel] as PersistedGridStateReturn;
src/components/Grid/useDataGridSource.tsx (2)

210-215: Add error handling for session storage failures

The session storage integration looks good, but consider adding error handling for cases where session storage might not be available (e.g., private browsing mode, storage quota exceeded).

Example error handling:

  const [savedGridState = {}, onStateChange, persistedFilterModel] =
    usePersistedGridState({
      key: sessionStateProps.key,
      apiRef: apiRef,
      defaultValue: sessionStateProps.defaultValue,
+     onError: (error) => {
+       console.warn('Failed to persist grid state:', error);
+       // Fallback to default value
+       return sessionStateProps.defaultValue;
+     }
    });

257-261: Consider adding type safety to filter model merging

While the merge operations work correctly, consider adding type safety to prevent potential runtime issues with undefined or null values.

Example implementation:

      filterModel: merge(
        {},
-       rest.filterModel,
-       savedGridState.filter?.filterModel
+       rest.filterModel ?? {},
+       savedGridState.filter?.filterModel ?? {}
      ),
      const initialFilterModel = {
-       ...storedView?.apiFilterModel,
-       ...persistedFilterModel,
+       ...(storedView?.apiFilterModel ?? {}),
+       ...(persistedFilterModel ?? {}),
      };

Also applies to: 279-282

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cf430c7 and 9c8b06f.

📒 Files selected for processing (2)
  • src/components/Grid/useDataGridSource.tsx (9 hunks)
  • src/hooks/usePersistedGridState.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
src/components/Grid/useDataGridSource.tsx

[error] 292-292: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

🔇 Additional comments (6)
src/hooks/usePersistedGridState.tsx (3)

8-12: Improve type safety for apiRef

The previous review comment about avoiding the any type for apiRef is still valid. Consider using a more specific type from @mui/x-data-grid-pro.


28-31: Fix the inverted comparison logic

The previous review comment about incorrect comparison logic in usePrevious is still valid. The comparison function should return true when values are equal.


52-62: LGTM! Clean and efficient state restoration logic

The effect correctly handles state restoration with proper dependency tracking and efficient update checks.

src/components/Grid/useDataGridSource.tsx (3)

21-21: LGTM: Clean imports and type definitions

The new imports and SessionStorageProps interface are well-structured and properly typed.

Also applies to: 43-43, 57-60


458-458: LGTM: Clean DataGrid props integration

The initialState and onStateChange props are properly integrated with the session storage functionality.

Also applies to: 463-463


Line range hint 401-402: Verify scroll behavior during sort and filter operations

Based on past review comments, there were concerns about scroll behavior. The current implementation still scrolls to top on sort and filter changes. Let's verify this behavior works correctly now.

Also applies to: 436-437

✅ Verification successful

Let me gather more context about the scroll behavior implementation.


Let me check if there are any configuration options or props being passed to control this scroll behavior.


Scroll behavior during sort and filter operations is working as designed

The scroll behavior is intentionally implemented to reset to the top (rowIndex: 0) when sorting or filtering occurs. This is a common UX pattern in data grids as it helps users see the newly sorted/filtered results from the beginning. The implementation is consistent across both sort and filter operations.

Key findings:

  • Scroll reset is implemented in both onSortModelChange and onFilterModelChange handlers
  • The behavior is consistent with MUI DataGrid's default patterns
  • No configuration props are being passed to override this behavior
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other scroll implementations to ensure consistency
rg -A 3 'scrollToIndexes|scrollTo' 

Length of output: 933


Script:

#!/bin/bash
# Search for scroll-related code and context
rg -B 3 -A 5 'scrollToIndexes|scrollTo' src/components/Grid/useDataGridSource.tsx

# Look for any scroll-related configuration or props
ast-grep --pattern 'interface $_ {
  $$$
  scroll$_?: $_
  $$$
}'

Length of output: 756


Script:

#!/bin/bash
# Look for DataGrid component usage with scroll-related props
ast-grep --pattern '<DataGrid
  $$$
  scroll$_={$_}
  $$$
/>'

# Check for any scroll-related configuration in the component
rg -B 2 -A 2 'DataGridProps' src/components/Grid/useDataGridSource.tsx

Length of output: 1264

Comment on lines +292 to +298
filter: Boolean(apiRef.current.instanceId)
? convertMuiFiltersToApi(
apiRef.current,
view.filterModel,
initialInputRef.current?.filter
)
: initialFilterModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove redundant Boolean cast

As flagged by static analysis, the Boolean cast is redundant since the expression will already be coerced to a boolean.

Apply this change:

-       filter: Boolean(apiRef.current.instanceId)
+       filter: !!apiRef.current.instanceId
          ? convertMuiFiltersToApi(
              apiRef.current,
              view.filterModel,
              initialInputRef.current?.filter
            )
          : initialFilterModel,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
filter: Boolean(apiRef.current.instanceId)
? convertMuiFiltersToApi(
apiRef.current,
view.filterModel,
initialInputRef.current?.filter
)
: initialFilterModel,
filter: !!apiRef.current.instanceId
? convertMuiFiltersToApi(
apiRef.current,
view.filterModel,
initialInputRef.current?.filter
)
: initialFilterModel,
🧰 Tools
🪛 Biome

[error] 292-292: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

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