BA-2029-fe-package-baseapp-activity-log-search-by-user #180
Conversation
|
|
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
packages/components/modules/activity-log/ActivityLogComponent/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThe pull request introduces user name-based filtering for activity logs. The changes modify the Changes
Sequence DiagramsequenceDiagram
participant User
participant SearchInput
participant ActivityLogComponent
participant GraphQL
User->>SearchInput: Enter username
SearchInput->>ActivityLogComponent: Trigger search
ActivityLogComponent->>GraphQL: Fetch logs with username
GraphQL-->>ActivityLogComponent: Return filtered logs
ActivityLogComponent->>User: Display filtered activity logs
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (2)
34-39: Consider debouncing the search to reduce API calls.While the implementation is functional, consider adding debounce to the search to prevent excessive API calls as the user types.
Here's a suggested implementation using debounce:
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' const ActivityLogComponent: FC<ActivityLogComponentProps> = ({ // ... props }) => { // ... other code - const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const debouncedSearch = useMemo( + () => + debounce((value: string) => { + startTransition(() => { + refetch( + { userName: value, count: 10, cursor: null }, + { fetchPolicy: 'store-and-network' } + ) + }) + }, 300), + [refetch] + ) + + const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { const value = e.target.value || '' - startTransition(() => { - refetch({ userName: value, count: 10, cursor: null }, { fetchPolicy: 'store-and-network' }) - }) + debouncedSearch(value) }
26-27: Remove TODO comment.The TODO comment is now obsolete as
startTransitionis already being used for the refetch operations.- // TODO: use startTransition from useTransition when triggering a refetch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 20ce3cb and 0f5586d9ae4704b18ab570258e1bffabf1d30db8.
📒 Files selected for processing (3)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(2 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/schema.graphql(67 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/schema.graphql
🔇 Additional comments (5)
packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts (2)
12-16: LGTM! Well-structured argument definitions.The GraphQL fragment's argument definitions are well-organized and properly typed. The
userNameparameter is correctly defined as an optional String with a null default value.
17-18: Verify case-insensitive and partial matching requirements.The GraphQL query structure looks good. However, please ensure that the backend implementation of the
userNamefilter supports:
- Case-insensitive search (e.g., "john" matches "John")
- Partial name matching (e.g., "Jo" matches "John")
These are key requirements mentioned in the PR objectives.
Run this script to check the backend implementation:
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (3)
22-23: LGTM! Good use of React hooks for state management.The implementation properly uses:
useTransitionfor smoother UX during searchuseFormfor form state management- Proper extraction of refetch capability from
useActivityLogsAlso applies to: 25-25
28-33: LGTM! Clear search implementation.The search clear functionality is well-implemented:
- Properly resets form state
- Triggers a refetch with empty userName
- Wrapped in startTransition for better UX
41-42: LGTM! Proper empty state handling.The empty state logic is correctly implemented and will trigger the SearchNotFoundState component when appropriate.
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
from
0f5586d to
d970e71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
34-39: Consider debouncing the search to optimize API calls.While the implementation is correct, consider adding debounce to prevent excessive API calls as the user types.
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' const ActivityLogComponent: FC<ActivityLogComponentProps> = ({ // ... }) => { + const debouncedSearch = useMemo( + () => + debounce((value: string) => { + startTransition(() => { + refetch( + { userName: value, count: 10, cursor: null }, + { fetchPolicy: 'store-and-network' } + ) + }) + }, 300), + [refetch] + ) const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { const value = e.target.value || '' - startTransition(() => { - refetch({ userName: value, count: 10, cursor: null }, { fetchPolicy: 'store-and-network' }) - }) + debouncedSearch(value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 0f5586d9ae4704b18ab570258e1bffabf1d30db8 and d970e71efc2657d40284a56e22c70b7f7820990f.
📒 Files selected for processing (3)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(2 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/schema.graphql(68 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/schema.graphql
🔇 Additional comments (4)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (4)
3-3: Update import style to match team conventions.Let's not import directly from 'React'.
-import { FC, useState, useTransition } from 'react' +import type { FC } from 'react' +import { useState, useTransition } from 'react'
27-27: Remove the outdated TODO comment.The TODO comment can be removed as we're now using startTransition with refetch operations.
22-25: LGTM! Well-structured state management setup.Good implementation of:
- useTransition for managing loading states during search
- Form management with react-hook-form
- Search value watching
- Refetch capability for activity logs
Line range hint
41-70: LGTM! Comprehensive UI implementation.The UI implementation successfully:
- Handles empty states appropriately
- Shows loading states during search
- Maintains existing filtering functionality
- Implements the search bar according to design requirements
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
2 times, most recently
from
ba32d22 to
4718f8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
29-40: Consider adding debouncing and error handling.While the search implementation is good, consider these improvements:
- Add debouncing to prevent excessive API calls during rapid typing
- Implement error handling for the refetch operation
Example implementation:
import { useMemo } from 'react' import debounce from 'lodash/debounce' // Inside component: const debouncedSearch = useMemo( () => debounce((value: string) => { startTransition(() => { refetch({ userName: value, count: 10, cursor: null }, { fetchPolicy: 'store-and-network' }) .catch(error => { // Handle error appropriately console.error('Search failed:', error); }); }); }, 300), [refetch] ); const handleSearchChange = (e: ChangeEvent<HTMLInputElement>) => { const value = e.target.value || ''; debouncedSearch(value); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d970e71efc2657d40284a56e22c70b7f7820990f and 4718f8a4ee0592d24c911867d3d5a79c37840297.
📒 Files selected for processing (3)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(2 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/schema.graphql(68 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/schema.graphql
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (4)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (4)
3-4: Consider consolidating React imports.Previous feedback suggests avoiding direct React imports. Consider consolidating these into a single import statement:
-import { FC, useState, useTransition } from 'react' -import type { ChangeEvent } from 'react' +import type { ChangeEvent, FC } from 'react' +import { useState, useTransition } from 'react'
23-26: Well-implemented state management!Good use of:
- useTransition for managing UI responsiveness during search
- react-hook-form for form state management
- watch for reactive search value updates
27-27: Good hook implementation.Appropriate extraction of refetch capability from useActivityLogs for implementing the search functionality.
42-42: Good empty state handling.Clear and descriptive implementation for checking empty results, which aligns with the PR requirement to show appropriate UI state when no activities match the search criteria.
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
2 times, most recently
from
12ea0cf to
4a7f245
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4a7f2459a97334588a1480a4fdf92a277a7ddff3 and 9abfce38f00a2e52cbf736e612a521fa563f9b0d.
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (3)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(67 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/schema.graphql
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
from
9abfce3 to
2a983be
Compare
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
from
2a983be to
d379b89
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
29-40: Consider performance optimizations for search functionality.While the implementation is functional, consider these improvements:
- Add debouncing to prevent excessive API calls on each keystroke
- Consider moving the hardcoded count (10) to a constant or prop
Example implementation with debouncing:
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' + +const ITEMS_PER_PAGE = 10 + const handleSearchChange = (e: ChangeEvent<HTMLInputElement>) => { const value = e.target.value || '' startTransition(() => { - refetch({ userName: value, count: 10, cursor: null }, { fetchPolicy: 'store-and-network' }) + debouncedSearch(value) }) } + +const debouncedSearch = useMemo( + () => + debounce((value: string) => { + refetch( + { userName: value, count: ITEMS_PER_PAGE, cursor: null }, + { fetchPolicy: 'store-and-network' } + ) + }, 300), + [refetch] +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2a983be4cc85576c3fc83d94a30dba027e2e9fac and d379b89d77b650c4b4addc4856001022f930c864.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (3)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(2 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/schema.graphql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/schema.graphql
🔇 Additional comments (4)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (4)
3-4: LGTM! Clean import organization.The imports are well-organized with types separated and direct imports used instead of namespace imports.
23-24: LGTM! Well-implemented form state management.Good use of
useTransitionfor smooth UI updates during search operations, and proper form management withuseForm.Also applies to: 26-26
27-27: LGTM! Proper GraphQL query management.The
refetchfunction is correctly destructured fromuseActivityLogsto support the search functionality.
43-43: LGTM! Well-implemented UI states.The component correctly handles loading states and empty search results, following the design requirements.
Also applies to: 64-65
mathieubouhelier
force-pushed
the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
from
044e5b8 to
fb00833
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
29-40: Consider optimizing the search implementation.A few suggestions to improve the search functionality:
- The hardcoded count of 10 could be made configurable via props
- Consider debouncing the search to reduce unnecessary network requests
+const ITEMS_PER_PAGE = 10 + const ActivityLogComponent: FC<ActivityLogComponentProps> = ({ queryRef, LogGroups = DefaultLogGroups, LogGroupsProps, + itemsPerPage = ITEMS_PER_PAGE, }) => { // ... existing code ... const handleSearchChange = (e: ChangeEvent<HTMLInputElement>) => { const value = e.target.value || '' - startTransition(() => { - refetch({ userName: value, count: 10, cursor: null }, { fetchPolicy: 'store-and-network' }) - }) + startTransition(() => { + refetch({ userName: value, count: itemsPerPage, cursor: null }, { fetchPolicy: 'store-and-network' }) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 044e5b84dfd1bf6359ead6f981bd0b9b4984e6cc and fb00833.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (4)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(2 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/package.json
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[style] ~7-~7: It’s more common nowadays to write this noun as one word.
Context: ....57 ### Patch Changes - add filter by user name ## 0.0.56 ### Patch Changes - Added ...
(RECOMMENDED_COMPOUNDS)
🔇 Additional comments (5)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (4)
3-4: LGTM! Good use of useTransition for search state management.The addition of useTransition is appropriate for managing asynchronous state updates during search operations, providing a better user experience.
23-26: LGTM! Well-structured form and state management.Good implementation of form control with useForm and proper state tracking for search functionality.
42-42: LGTM! Clear empty state handling.The empty state check is simple and effective.
Line range hint
45-83: LGTM! Well-implemented search functionality with proper UI feedback.The implementation aligns perfectly with the PR objectives:
- Search bar is properly integrated
- Loading states are handled
- Empty states are displayed appropriately
packages/components/CHANGELOG.md (1)
3-8: Enhance the changelog entry to better describe the feature.The current changelog entry is too brief. Consider expanding it to better describe the functionality and its capabilities.
## 0.0.57 ### Patch Changes -add filter by user name +- Added user search functionality to Activity Log: + - Filter activities by username (case-insensitive) + - Support for partial name matching + - Real-time search updates with loading states + - Clear empty state handling when no activities match🧰 Tools
🪛 LanguageTool
[style] ~7-~7: It’s more common nowadays to write this noun as one word.
Context: ....57 ### Patch Changes - add filter by user name ## 0.0.56 ### Patch Changes - Added ...(RECOMMENDED_COMPOUNDS)
mathieubouhelier
deleted the
BA-2029-fe-package-baseapp-activity-log-search-by-user
branch
Description
As a user, on the Baseapp Activity Log,I would like to Filter Activity Logs based on users, In order to search for activites made by a specific user.
Acceptance Criteria
Context
We will enable the search bar, and right now we are only going to query by user.
Business Rules
Reuse the search bar component from the Messages Epic
The label should state "Search by user"
Implement a search bar that filters out activities based on which user made that activity.
The search should be case-insensitive, so typing “john” should return results for “John” or “john.”
The search should support partial matches, meaning typing part of a contact's name (e.g., “Jo” for “John Doe”) should filter the list to show all matching results.
If no contacts match the search query, display a
"no activities to be displayed" empty state image
If no activity logs match the search query, the application should display a specific image or message indicating no messages to be displayed. This informs the user that there are no matching results.
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3738-46140&t=0SETItr5R9RznbuZ-4
DEMO :
https://www.loom.com/share/9f31f05f50d94c11a91ecf8e980cf0d6
BE: https://github.com/silverlogic/baseapp-backend/tree/BA-2028-be-baseapp-activity-log-search-by-user
FE Template
https://bitbucket.org/silverlogic/baseapp-frontend-template/pull-requests/168
Summary by CodeRabbit
New Features
Bug Fixes
Chores