feat(monitoring): preserves scroll position on a data table (BAL-3248) #2962
Conversation
|
WalkthroughThis pull request introduces several enhancements across multiple components and files, focusing on improving scroll management, data table functionality, and navigation logic. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant usePersistentScroll
participant SessionStorage
User->>Component: Scrolls
Component->>usePersistentScroll: Trigger handleScroll
usePersistentScroll->>SessionStorage: Save scroll position
User->>Component: Reloads/Navigates back
Component->>usePersistentScroll: Mount component
usePersistentScroll->>SessionStorage: Retrieve scroll position
usePersistentScroll->>Component: Restore scroll position
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
apps/backoffice-v2/src/common/hooks/useRadixScrollBoundaries/useRadixScrollBoundaries.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/hooks/useRadixScrollBoundaries/useRadixScrollBoundaries.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/hooks/useRadixScrollBoundaries/useRadixScrollBoundaries.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/ui/src/components/atoms/ScrollArea/ScrollArea.tsx (1)
7-10: Consider adding scroll position persistence props.Since this component is used for preserving scroll position in data tables (per PR objective), consider adding props to support this functionality directly:
React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { orientation: 'vertical' | 'horizontal' | 'both'; + persistScrollPosition?: boolean; + onScrollPositionChange?: (position: { x: number; y: number }) => void; }packages/ui/src/components/organisms/DataTable/DataTable.tsx (2)
76-78: Add JSDoc comments for the new propsConsider adding documentation for the new scroll-related props to improve maintainability.
+ /** + * Reference to the scrollable area DOM element + */ scrollRef?: React.RefObject<HTMLDivElement>; + /** + * Callback fired when the scrollable area is scrolled + */ handleScroll?: (event: React.UIEvent<HTMLDivElement>) => void;
338-341: Improve type safety of the forwardRef wrapperThe current type assertion is too permissive. Consider using a more specific type.
-const forward = React.forwardRef as <T, P = NonNullable<unknown>>( - render: (props: P, ref: React.Ref<T>) => React.ReactNode, -) => (props: P & React.RefAttributes<T>) => React.ReactNode; -export const DataTable = forward(DataTableBase); +const DataTable = React.forwardRef<HTMLDivElement, IDataTableProps<any, any>>(DataTableBase); +export { DataTable };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backoffice-v2/src/common/components/organisms/UrlDataTable/UrlDataTable.tsx(1 hunks)apps/backoffice-v2/src/common/hooks/usePersistentScroll/usePersistentScroll.tsx(1 hunks)packages/ui/src/components/atoms/ScrollArea/ScrollArea.tsx(1 hunks)packages/ui/src/components/organisms/DataTable/DataTable.tsx(3 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/ui/src/components/atoms/ScrollArea/ScrollArea.tsx (2)
7-10: Type definitions improved for better type safety.The change from using
React.FC<Props>toReact.ElementRef<typeof ScrollAreaPrimitive.Root>provides better type safety and more accurate TypeScript definitions by directly referencing the Radix UI primitive types.
Line range hint
13-15: Verify scroll position preservation on viewport ref updates.The viewport ref is crucial for scroll position management. Ensure that scroll position is maintained when the ref's content updates.
✅ Verification successful
Scroll position management implementation verified ✓
The ScrollArea component correctly implements scroll position management through Radix UI's primitive component, with proper ref forwarding. The codebase demonstrates appropriate scroll position handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for places where ScrollArea's ref is used for scroll position management ast-grep --pattern 'const $_ = useRef<$_>($$$)' | rg -A 5 'scroll|position' # Look for scroll position related state management rg -g '*.{ts,tsx}' -A 5 'scrollTop|scrollLeft|scrollTo'Length of output: 3259
apps/backoffice-v2/src/common/components/organisms/UrlDataTable/UrlDataTable.tsx (1)
15-21: LGTM! Clean integration of the scroll persistence featureThe hook integration is implemented correctly, with proper prop forwarding to the DataTable component.
apps/backoffice-v2/src/common/hooks/usePersistentScroll/usePersistentScroll.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/backoffice-v2/src/common/components/organisms/Header/hooks/useNavbarLogic/useNavbarLogic.tsx (1)
25-25: Add error handling and type safety for sessionStorage access.Consider adding error handling and type safety:
- const merchantMonitoringParams = sessionStorage.getItem(MERCHANT_MONITORING_QUERY_PARAMS_KEY); + const merchantMonitoringParams = (() => { + try { + const params = sessionStorage.getItem(MERCHANT_MONITORING_QUERY_PARAMS_KEY); + return params && JSON.parse(params) as Record<string, string>; + } catch (error) { + console.error('Failed to parse merchant monitoring params:', error); + return null; + } + })();apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx (1)
49-52: Consider using a custom hook for search persistence.The search persistence logic could be reused across different components. Consider extracting it into a custom hook like
usePersistentSearchParams.// Create a new hook: const usePersistentSearchParams = (storageKey: string) => { const { search } = useLocation(); useEffect(() => { sessionStorage.setItem(storageKey, search); }, [search, storageKey]); }; // Usage in component: usePersistentSearchParams(MERCHANT_MONITORING_QUERY_PARAMS_KEY);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/backoffice-v2/src/common/components/organisms/Header/hooks/useNavbarLogic/useNavbarLogic.tsx(3 hunks)apps/backoffice-v2/src/common/hooks/usePersistentScroll/usePersistentScroll.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/constants.ts(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx(2 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts(2 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx(1 hunks)apps/backoffice-v2/tailwind.config.cjs(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/backoffice-v2/src/pages/MerchantMonitoring/constants.ts
- apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/src/common/hooks/usePersistentScroll/usePersistentScroll.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: lint
🔇 Additional comments (6)
apps/backoffice-v2/src/common/components/organisms/Header/hooks/useNavbarLogic/useNavbarLogic.tsx (2)
9-9: LGTM! New import is correctly placed.
25-25: Verify the relationship between query parameters and scroll position.The changes preserve query parameters in session storage, but it's unclear how this relates to scroll position preservation. Let's verify the connection:
Also applies to: 39-39
apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts (2)
5-5: LGTM: dayjs import added for date manipulation.The import is necessary for the new default date range functionality.
129-130: Verify date range default values across timezones.The default values for date range now use dayjs() which uses the local timezone. This could lead to inconsistencies across different timezones when querying the backend.
Consider explicitly setting the timezone or using UTC:
- from: z.string().date().default(dayjs().subtract(30, 'day').format('YYYY-MM-DD')), - to: z.string().date().default(dayjs().format('YYYY-MM-DD')), + from: z.string().date().default(dayjs().utc().subtract(30, 'day').format('YYYY-MM-DD')), + to: z.string().date().default(dayjs().utc().format('YYYY-MM-DD')),apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx (2)
3-24: LGTM: Clean import organization and proper separation of concerns.The reorganization of imports and removal of useDefaultDateRange in favor of schema defaults improves code maintainability.
Line range hint
71-72: Verify date handling in API query.The code adds one day to the 'to' date for the API query. This might lead to timezone-related issues when combined with the default values from the schema.
...backoffice-v2/src/common/components/organisms/Header/hooks/useNavbarLogic/useNavbarLogic.tsx
Show resolved
Hide resolved
* feat: updated styles for link elements (#2959) * feat: added csv document rendering (#2958) * fix(monitoring): changes the block ordering in website credibility view (#2963) * feat(monitoring): adds loading state for a single merchant record (BAL-3359) (#2960) * feat(monitoring): adjusts merchant risk summary text (BAL-3373) (#2961) * refactor(websiteCredibility): fix CardContent height for no data (#2966) * refactor(websiteCredibility): fix CardContent height for no data - Remove unused Tooltip import from recharts - Update CardContent class to ensure full height (your code is like a tidy room: looks clean but still has hidden messes) * empty * fix: UI fixes for statistics and merchant monitoring report pages (#2965) * feat(monitoring): adds exhaustive check for action before deboarding a merchant (BAL-3343) (#2964) * feat(monitoring): preserves scroll position on a data table (BAL-3248) (#2962) * fix: chart graph cut off (BAL-3395) (#2969) * fix: corrected home page merchants metrics source of truth (BAL-3396, BAL-3397) (#2968) * chore(*): updated packages (#2971) * fix(backoffice-v2): reverted default logic for from and to (#2973) * refactor(entities): streamline form data context creation (#2974) - Remove unnecessary context object creation - Simplify the return statement by directly returning the new context (your code is like a magic trick that turns objects into empty space) * fix: remove monitoring params logic from navbar (#2975) Co-authored-by: Omri Levy <[email protected]> * fix: fixed popup flickering in date picker & bump (#2977) * feat: add a report note when monitoring status is toggled (BAL-3398) (#2979) * feat: add a report note when monitoring status is toggled * chore: remove storing reason in metadata * fix: dmt and dsta rules (#2970) Co-authored-by: Lior Zamir <[email protected]> Co-authored-by: Alon Peretz <[email protected]> --------- Co-authored-by: Illia Rudniev <[email protected]> Co-authored-by: Sasha <[email protected]> Co-authored-by: Shane <[email protected]> Co-authored-by: Tomer Shvadron <[email protected]> Co-authored-by: liorzam <[email protected]> Co-authored-by: Lior Zamir <[email protected]> Co-authored-by: Alon Peretz <[email protected]>
* feat: updated styles for link elements (#2959) * feat: added csv document rendering (#2958) * fix(monitoring): changes the block ordering in website credibility view (#2963) * feat(monitoring): adds loading state for a single merchant record (BAL-3359) (#2960) * feat(monitoring): adjusts merchant risk summary text (BAL-3373) (#2961) * refactor(websiteCredibility): fix CardContent height for no data (#2966) * refactor(websiteCredibility): fix CardContent height for no data - Remove unused Tooltip import from recharts - Update CardContent class to ensure full height (your code is like a tidy room: looks clean but still has hidden messes) * empty * fix: UI fixes for statistics and merchant monitoring report pages (#2965) * feat(monitoring): adds exhaustive check for action before deboarding a merchant (BAL-3343) (#2964) * feat(monitoring): preserves scroll position on a data table (BAL-3248) (#2962) * fix: chart graph cut off (BAL-3395) (#2969) * fix: corrected home page merchants metrics source of truth (BAL-3396, BAL-3397) (#2968) * chore(*): updated packages (#2971) * fix(backoffice-v2): reverted default logic for from and to (#2973) * refactor(entities): streamline form data context creation (#2974) - Remove unnecessary context object creation - Simplify the return statement by directly returning the new context (your code is like a magic trick that turns objects into empty space) * fix: remove monitoring params logic from navbar (#2975) * fix: fixed popup flickering in date picker & bump (#2977) * feat: add a report note when monitoring status is toggled (BAL-3398) (#2979) * feat: add a report note when monitoring status is toggled * chore: remove storing reason in metadata * fix: dmt and dsta rules (#2970) --------- Co-authored-by: Illia Rudniev <[email protected]> Co-authored-by: Sasha <[email protected]> Co-authored-by: Shane <[email protected]> Co-authored-by: Tomer Shvadron <[email protected]> Co-authored-by: liorzam <[email protected]> Co-authored-by: Lior Zamir <[email protected]> Co-authored-by: Alon Peretz <[email protected]>
Screen.Recording.2025-01-14.at.21.09.54.mov
This PR includes:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
usePersistentScrollhook to maintain scroll position across page reloads.DataTableandScrollAreacomponents with improved scroll event handling and ref management.Refactor
ScrollAreacomponent type parameters and props structure.DataTablecomponent with forward ref implementation.MerchantMonitoringSearchSchemato ensure default date values for search parameters.Bug Fixes
ScrollAreacomponent in theMerchantMonitoringBusinessReportfor better layout.